gianm commented on a change in pull request #10027:
URL: https://github.com/apache/druid/pull/10027#discussion_r440572205
##########
File path:
processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java
##########
@@ -141,34 +142,38 @@ public ChainedExecutionQueryRunner(
);
}
)
- )
- );
+ );
- queryWatcher.registerQueryFuture(query, futures);
+ Function<Throwable, Void> cancelFunction = (t) -> {
Review comment:
This little block is used in a few places, and it would be good to
include comments about why it's needed.
Could you please move it to GuavaUtils and also make the following changes:
- Add a comment about why it's needed: it's an alternative to
`Futures.allAsList(...).cancel`, that is necessary because `Futures.allAsList`
creates a Future that cannot be canceled if one of its constituent futures has
already failed. (This comment is the real reason that it's good to have it be
its own function.)
- The function isn't doing anything with the Throwable, so it could just be
`void cancelAll(List<Future<T>>)`.
- Add unit tests to GuavaUtilsTest.
Then we can do stuff like `GuavaUtils.cancelAll(futures)`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
