Skip to content

Commit

Permalink
ScanQueryFrameProcessor: Close CursorHolders as we go along. (#17152)
Browse files Browse the repository at this point in the history
* ScanQueryFrameProcessor: Close CursorHolders as we go along.

The change in #16533 added CursorHolders to the processor-level Closer.
This is problematic when running on an input channel: it means we started
keeping around all CursorHolders for all frames we process and closing them
when the channel is complete, rather than closing them as we go along.

This patch restores the prior behavior, where resources are closed as we go.

* Fix other call sites.

* Fix reference.

* Improvements.
  • Loading branch information
gianm authored Sep 25, 2024
1 parent 203d634 commit 2e531bb
Showing 1 changed file with 24 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ public class ScanQueryFrameProcessor extends BaseLeafFrameProcessor
private final Closer closer = Closer.create();

private Cursor cursor;
private Closeable cursorCloser;
private Segment segment;
private final SimpleSettableOffset cursorOffset = new SimpleAscendingOffset(Integer.MAX_VALUE);
private FrameWriter frameWriter;
Expand Down Expand Up @@ -156,6 +157,7 @@ public ReturnOrAwait<Object> runIncrementally(final IntSet readableInputs) throw
@Override
public void cleanup() throws IOException
{
closer.register(cursorCloser);
closer.register(frameWriter);
closer.register(super::cleanup);
closer.close();
Expand Down Expand Up @@ -221,7 +223,7 @@ protected ReturnOrAwait<SegmentsInputSlice> runWithDataServerQuery(final DataSer
cursorYielder.close();
return ReturnOrAwait.returnObject(handedOffSegments);
} else {
final long rowsFlushed = setNextCursor(cursorYielder.get(), null);
final long rowsFlushed = setNextCursor(cursorYielder.get(), null, null);
closer.register(cursorYielder);
if (rowsFlushed > 0) {
return ReturnOrAwait.runAgain();
Expand Down Expand Up @@ -256,16 +258,16 @@ protected ReturnOrAwait<Unit> runWithSegment(final SegmentWithDescriptor segment
);
}

final CursorHolder cursorHolder = closer.register(
cursorFactory.makeCursorHolder(ScanQueryEngine.makeCursorBuildSpec(query, null))
);
final Cursor nextCursor = cursorHolder.asCursor();
final CursorHolder nextCursorHolder =
cursorFactory.makeCursorHolder(ScanQueryEngine.makeCursorBuildSpec(query, null));
final Cursor nextCursor = nextCursorHolder.asCursor();

if (nextCursor == null) {
// No cursors!
nextCursorHolder.close();
return ReturnOrAwait.returnObject(Unit.instance());
} else {
final long rowsFlushed = setNextCursor(nextCursor, segmentHolder.get().getSegment());
final long rowsFlushed = setNextCursor(nextCursor, nextCursorHolder, segmentHolder.get().getSegment());
assert rowsFlushed == 0; // There's only ever one cursor when running with a segment
}
}
Expand Down Expand Up @@ -302,16 +304,16 @@ protected ReturnOrAwait<Unit> runWithInputChannel(
);
}

final CursorHolder cursorHolder = closer.register(
cursorFactory.makeCursorHolder(ScanQueryEngine.makeCursorBuildSpec(query, null))
);
final Cursor nextCursor = cursorHolder.asCursor();
final CursorHolder nextCursorHolder =
cursorFactory.makeCursorHolder(ScanQueryEngine.makeCursorBuildSpec(query, null));
final Cursor nextCursor = nextCursorHolder.asCursor();

if (nextCursor == null) {
// no cursor
nextCursorHolder.close();
return ReturnOrAwait.returnObject(Unit.instance());
}
final long rowsFlushed = setNextCursor(nextCursor, frameSegment);
final long rowsFlushed = setNextCursor(nextCursor, nextCursorHolder, frameSegment);

if (rowsFlushed > 0) {
return ReturnOrAwait.runAgain();
Expand Down Expand Up @@ -415,10 +417,20 @@ private long flushFrameWriter() throws IOException
}
}

private long setNextCursor(final Cursor cursor, final Segment segment) throws IOException
private long setNextCursor(
final Cursor cursor,
@Nullable final Closeable cursorCloser,
final Segment segment
) throws IOException
{
final long rowsFlushed = flushFrameWriter();
if (this.cursorCloser != null) {
// Close here, don't add to the processor-level Closer, to avoid leaking CursorHolders. We may generate many
// CursorHolders per instance of this processor, and we need to close them as we go, not all at the end.
this.cursorCloser.close();
}
this.cursor = cursor;
this.cursorCloser = cursorCloser;
this.segment = segment;
this.cursorOffset.reset();
return rowsFlushed;
Expand Down

0 comments on commit 2e531bb

Please sign in to comment.