gortiz opened a new pull request, #19864: URL: https://github.com/apache/pinot/pull/19864
## Summary `HardLimitExecutor` checked its limit twice: on submit and again inside the wrapper, right before the task ran. The second check could reject a task that passed the first one. When that happened, the task never ran and nobody was told. This PR keeps the submit-time check as the only rejection point and adds regression tests. ## The problem The wrapper that `decorate` returns called `checkTaskAllowed()` before it incremented the running counter. If another task started between the submit-time check and the start of this task, the second check threw `IllegalStateException`. For the multi-stage engine this had three effects: - `OpChainSchedulerService` submits a `ListenableFutureTask` and reads only its completion callback. The wrapper threw before the task ran, so the callback never fired. The opchain was never closed and the per-request active counter never reached zero. - The downstream receiver waited for a block that never came, until the query timed out. - `AbstractExecutorService.submit` wraps the task in a `FutureTask`. The exception was captured in that future. The scheduler does not read it, so nothing was logged. The default configuration does not set a hard limit, so a deployment is only affected when `pinot.server.query.executor.mse.max.execution.threads` (or the cluster factor) is set with the `ERROR` strategy. ## The fix `decorate` keeps the synchronous check. The wrapper now only increments the running counter, runs the task, and decrements the counter. A task that passes the submit-time check always runs. The counter can exceed the limit by the number of tasks that were accepted but had not started when a new task was checked. On a cached pool this window is negligible. Reserving the slot at submit time was considered and rejected: a task that is cancelled before it starts would then hold its slot forever. ## Tests - `HardLimitExecutorTest.testAcceptedTaskRunsEvenIfLimitIsReachedBeforeItStarts` uses a deferred executor that controls when each task starts. It submits two tasks, starts the first, and makes sure that the second still runs. A third submit is rejected synchronously. - `HardLimitExecutorOpChainRegressionTest` drives `OpChainSchedulerService` through the same race and makes sure that both opchains run, close, and release the request. Both tests failed before the change (the second task was dropped and the request stayed active) and pass after it. -- 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]
