gortiz commented on code in PR #20099:
URL: https://github.com/apache/pinot/pull/20099#discussion_r4427156538
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -905,7 +905,9 @@ private BrokerResponse query(QueryEnvironment.CompiledQuery
query, long requestI
// Short-circuit: if all leaf stages are empty (all segments pruned or
table has no data),
// run only the broker reduce stage locally. No server dispatch is
attempted.
boolean allLeafStagesEmpty = dispatchableSubPlan.isAllLeafStagesEmpty();
- int estimatedNumQueryThreads =
getEstimatedNumQueryThreads(dispatchableSubPlan);
+ // A query estimated above the throttler limit takes the whole limit, so
it runs alone instead of never
+ int estimatedNumQueryThreads =
+ Math.min(getEstimatedNumQueryThreads(dispatchableSubPlan),
_queryThrottler.getMaxPermits());
Review Comment:
+1. Moving the cap into the throttler also fixes the race between
`getMaxPermits()` and `tryAcquire()` in the handler. With the cap in the
throttler, the `LOG` strategy can keep the uncapped estimate, so it still shows
how large the queries really are. I am fine with doing this in a follow-up PR.
--
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]
