jtuglu1 commented on code in PR #18148:
URL: https://github.com/apache/druid/pull/18148#discussion_r2357320740
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java:
##########
@@ -392,6 +403,14 @@ private void waitForFutureCompletion(
}
catch (ExecutionException e) {
GuavaUtils.cancelAll(true, future, futures);
+ Throwable cause = e.getCause();
+ if (cause instanceof TimeoutException || cause instanceof
QueryTimeoutException) {
Review Comment:
> I wonder if a simpler approach could be to use the same timeout we already
have, but shorten it in `SpecificSegmentQueryRunner`.
I'm not sure this will work. The reason being is that while this does
execute per segment, the timeout is not enforced here. The timeout is
unfortunately enforced in the caller runners which aggregate lists of
per-segment runners into callable futures (e.g. `ChainedExecutionQueryRunner`
or `GroupByMergingQueryRunner`). These callers are in higher stack frames
whereas the SpecificSegmentQueryRunner is near the bottom of the stack. The
thread that is running the SpecificSegmentQueryRunner is the same thread that
we'd be asking to service the timer interrupt to cancel the work(which won't
work in Java AFAIK since the Futures framework is not truly async). These
callers rely on the aggregate query timeout to make one mega future with that
value.
--
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]
