gianm commented on code in PR #18148:
URL: https://github.com/apache/druid/pull/18148#discussion_r2350333749
##########
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:
It's not ideal that the query runners all need special handling for the
per-segment timeouts. It seems brittle. What happens if a query runner doesn't
implement it properly, or at all?
I wonder if a simpler approach could be to use the same timeout we already
have, but shorten it in `SpecificSegmentQueryRunner`. i.e., update
`SpecificSegmentQueryRunner` to set the `timeout` in the query context to
`min(timeout, perSegmentTimeout)` when it runs the query on the individual
segment. I think this should work just as well, but would require fewer/no
changes to the query engines.
##########
processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java:
##########
@@ -165,8 +180,16 @@ public Iterable<T> call()
}
catch (ExecutionException e) {
GuavaUtils.cancelAll(true, future, futures);
- Throwables.propagateIfPossible(e.getCause());
- throw new RuntimeException(e.getCause());
+ Throwable cause = e.getCause();
+ Throwables.propagateIfPossible(cause);
+ if (cause instanceof TimeoutException || cause instanceof
QueryTimeoutException) {
Review Comment:
I think this can't ever be `QueryTimeoutException`, because that's a
`RuntimeException`, and `Throwables.propagateIfPossible(cause)` would have
thrown it.
##########
server/src/main/java/org/apache/druid/guice/DruidProcessingModule.java:
##########
@@ -157,6 +158,7 @@ public static QueryProcessingPool
createProcessingExecutorPool(
lifecycle,
config
),
+ ScheduledExecutors.fixed(config.getNumTimeoutThreads(),
"PrioritizedExecutorService-Timeout-%%d"),
Review Comment:
Let's make this opt-in, so if `numTimeoutThreads = 0` (which should be the
default) there should be no timeout executor created, and per-segment timeouts
should be ignored.
--
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]
