Copilot commented on code in PR #19167:
URL: https://github.com/apache/pinot/pull/19167#discussion_r3724461303
##########
pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/SecondaryWorkloadQueue.java:
##########
@@ -158,4 +170,14 @@ private void
checkSchedulerGroupCapacity(SchedulerQueryContext query)
+ _resourceManager.getTableThreadsHardLimit());
}
}
+
+ /// Signals the reader that reserved threads were released, so canSchedule()
may now pass.
+ public void signalWorkersReleased() {
+ _queueLock.lock();
+ try {
+ _queryReaderCondition.signal();
+ } finally {
+ _queueLock.unlock();
+ }
+ }
Review Comment:
`signalWorkersReleased()` unconditionally signals the condition even when
the queue is empty. That can cause unnecessary wakeups of the scheduler thread
(it will wake up, observe an empty queue, then go back to waiting). Guarding
the signal avoids reintroducing avoidable wakeups while keeping the intended
behavior when there are pending queries blocked on `canSchedule()`.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/BinaryWorkloadScheduler.java:
##########
@@ -157,6 +158,7 @@ public void run() {
public void run() {
executorService.releaseWorkers();
schedulerGroup.endQuery();
+ _secondaryQueryQ.signalWorkersReleased();
_secondaryRunnerSemaphore.release();
checkStopResourceManager();
Review Comment:
If `SecondaryWorkloadQueue.take()` returns null (e.g. when `stop()`
interrupts the scheduler thread while it is blocked in `take()`), the loop
`continue`s without releasing the permit already acquired from
`_secondaryRunnerSemaphore`. This can prevent `checkStopResourceManager()` from
ever observing all permits available and may leave the ResourceManager running
after shutdown.
--
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]
