gianm commented on code in PR #19397:
URL: https://github.com/apache/druid/pull/19397#discussion_r3176204717
##########
multi-stage-query/src/main/java/org/apache/druid/msq/querykit/scan/ScanQueryFrameProcessor.java:
##########
@@ -297,21 +304,29 @@ protected ReturnOrAwait<SegmentsInputSlice>
runWithDataServerQuery(final DataSer
protected ReturnOrAwait<Unit> runWithSegment(final SegmentReferenceHolder
segmentHolder) throws IOException
{
if (cursor == null) {
- final Segment segment = mapSegment(segmentHolder, closer);
- final CursorFactory cursorFactory = segment.as(CursorFactory.class);
- if (cursorFactory == null) {
- throw DruidException.defensive(
- "Null cursor factory found. Probably trying to issue a query
against a segment being memory unmapped."
+ if (cursorHolderFuture == null) {
+ final Segment segment = mapSegment(segmentHolder, closer);
+ final CursorFactory cursorFactory = segment.as(CursorFactory.class);
+ if (cursorFactory == null) {
+ throw DruidException.defensive(
+ "Null cursor factory found. Probably trying to issue a query
against a segment being memory unmapped."
+ );
+ }
+
+ cursorHolderFuture = cursorFactory.makeCursorHolderAsync(
Review Comment:
Handling futures that return closeable things is tricky. Maybe we can
improve it by changing the return of `makeCursorHolderAsync` from
`ListenableFuture<CursorHolder>` to `AsyncCursorHolder` that is closeable and
has methods `get()` (blocks if not ready), `close()` (closes the resource no
matter where it is in its lifecycle), and `addReadyCallback(Runnable)` (used by
nonblocking callers to learn when `get` is ready).
The problem with the future approach is that once this call site gets the
`cursorHolderFuture`, it's responsible for monitoring the future and closing
`cursorHolder` if the future resolves successfully. This has to be done even if
the processor is canceled before it has a chance to run through normally. It
requires extra carefulness and is easy to mess up.
One way it can be handled is by attaching a callback in `cleanup` that
closes the holder in `onSuccess`, like:
```
if (cursorHolderFuture != null) {
Futures.addCallback(
cursorHolderFuture,
new FutureCallback<>() {
void onSuccess(CursorHolder holder) { holder.close(); }
void onFailure(Throwable t) { /* nothing */ }
}
);
}
```
But even with this structure, it's important watch out for pitfalls. A big
one is that you can never cancel a future that returns a closeable thing.
Cancellation of the future can cause the object to be orphaned and eventually
GCed without being closed (if the object is created before cancellation has a
chance to interrupt whatever was creating it).
If we can avoid these problems by returning something *directly* closeable
(like this `AsyncCursorHolder` idea) rather than future-of-closeable, then the
caller code becomes simpler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
