xiangfu0 commented on code in PR #20099:
URL: https://github.com/apache/pinot/pull/20099#discussion_r4419200089
##########
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:
**MAJOR: a capped query blocks every query behind it when the pool
shrinks.** A capped query asks for exactly the current pool. If the pool then
gets smaller while it waits, it can never be granted. The semaphore is fair, so
every later MSE query on this broker queues behind it until it times out,
including queries that would fit. The pool shrinks when a broker is added, a
server is removed, or (new with this PR) a table is created on a broker tenant
that served no table. When that happens, every broker's pool shrinks at once.
Repro with the PR's throttler: pool 4. One 1-thread query runs, and a query
estimated at 10 (capped to 4) waits. A third broker starts serving a table, so
the pool drops to 2, and the running query releases. A 1-thread query with 2
free permits then times out after 1s. It succeeds only after the capped query
times out. On master, the estimate-10 query fails fast and the 1-thread query
gets in at once. Master only stalls when an estimate falls between the new and
old pool sizes. With the cap, every oversize query hits this case.
One option: re-cap inside `tryAcquire`. Wait in short timed slices, re-read
`getTotalPermits()` on each slice, and return the number of permits acquired so
the caller releases that number. Please also add a test that shrinks the pool
while a capped query waits.
--
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]
