Fix missing expired query callbacks and improve secondary workload queue thread wakeup - #19167
Open
adasari wants to merge 2 commits into
Open
Fix missing expired query callbacks and improve secondary workload queue thread wakeup#19167adasari wants to merge 2 commits into
adasari wants to merge 2 commits into
Conversation
…eue thread wakeup
Contributor
Author
|
@yashmayya Could you review when you get a chance ? Please feel free to re-assign. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the secondary-query scheduling path in pinot-core by (1) reducing idle wakeups in the secondary workload dispatch loop and (2) ensuring expired secondary queries are completed via a callback so their response futures don’t leak.
Changes:
- Update
SecondaryWorkloadQueue.take()to do an untimed wait when empty, and to return expired queries fromtrimExpired()for callback-based completion outside the queue lock. - Extend
SchedulerGroup.trimExpired()to return the removed queries, enabling callers to complete their futures. - Wire expiry handling + worker-release signaling into
BinaryWorkloadScheduler, and add unit tests validating ordering, expiry completion, and wakeup behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pinot-core/src/test/java/org/apache/pinot/core/query/scheduler/SecondaryWorkloadQueueTest.java | New unit tests covering expiry completion, lock behavior, and wakeup signaling semantics. |
| pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/SecondaryWorkloadQueue.java | Adds expiry callback handling, eliminates idle timed polling when the queue is empty, and adds signalWorkersReleased(). |
| pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/SchedulerGroup.java | Changes trimExpired() to return removed queries for completion by callers. |
| pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/BinaryWorkloadScheduler.java | Passes expiry handler into SecondaryWorkloadQueue and signals when workers are released. |
| pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/AbstractSchedulerGroup.java | Implements trimExpired() returning the removed expired queries. |
Comment on lines
159
to
163
| executorService.releaseWorkers(); | ||
| schedulerGroup.endQuery(); | ||
| _secondaryQueryQ.signalWorkersReleased(); | ||
| _secondaryRunnerSemaphore.release(); | ||
| checkStopResourceManager(); |
Contributor
Author
There was a problem hiding this comment.
Added query completion callback functionality when take return null i.e no query.
Comment on lines
+174
to
+182
| /// Signals the reader that reserved threads were released, so canSchedule() may now pass. | ||
| public void signalWorkersReleased() { | ||
| _queueLock.lock(); | ||
| try { | ||
| _queryReaderCondition.signal(); | ||
| } finally { | ||
| _queueLock.unlock(); | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
not worth changing? if the queue is empty, a waiting thread wakes, reacquires the lock, sees the queue is empty, and waits again.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19167 +/- ##
============================================
+ Coverage 66.57% 66.60% +0.03%
Complexity 1423 1423
============================================
Files 3441 3441
Lines 218320 218365 +45
Branches 34737 34745 +8
============================================
+ Hits 145341 145437 +96
+ Misses 61251 61205 -46
+ Partials 11728 11723 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jackie-Jiang
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problems:
BinaryWorkloadSchedulerdispatch thread polls SecondaryWorkloadQueue every 1 ms (default) regardless of whether any secondary queries exist. Secondary queries are not frequent, So on a server with no secondary queries, there are ~1000 needless wakeups per min, per server, on a Thread.MAX_PRIORITY daemon thread.Changes
BinaryWorkloadScheduleronly to begin with,PriorityScheduleris untouched)SecondaryWorkloadQueueonly to begin with,MultiLevelPriorityQueueis untouched)Testing:
Unit tests
SecondaryWorkloadQueueTest