-
Notifications
You must be signed in to change notification settings - Fork 847
fix: fix scheduler deadlock when reducing max_outstanding_requests_per_tenant #7188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…urs when a tenant's max_outstanding_requests_per_tenant limit is dynamically reduced via runtime configuration. When MaxOutstandingPerTenant is reduced while a user's FIFORequestQueue is full, the getOrAddQueue method attempts to migrate requests to a smaller queue. Previously, this loop blocked indefinitely when the new queue capacity was reached, causing the scheduler to freeze. The fix ensures the migration loop breaks when the new queue is full, effectively dropping excess requests instead of blocking. Signed-off-by: Kishore K G <[email protected]>
Signed-off-by: Kishore K G <[email protected]>
Signed-off-by: Kishore K G <[email protected]>
|
@SungJin1212 can you please review this PR when you get a chance ? |
|
|
||
| // flush to new queue | ||
| for uq.queue.length() > 0 { | ||
| for (uq.queue.length() > 0) && (tmpQueue.length() < maxOutstanding) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add some comment to this line?
like
| for (uq.queue.length() > 0) && (tmpQueue.length() < maxOutstanding) { | |
| // If the new limit is lower than the current number of requests, | |
| // the excess requests (newest ones) will be dropped to prevent deadlocks. | |
| for (uq.queue.length() > 0) && (tmpQueue.length() < maxOutstanding) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
SungJin1212
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me, I left one comment. Thanks for contribution!
|
@kishorekg1999 |
Signed-off-by: Kishore K G <[email protected]>
|
Thanks for the quick response @SungJin1212 ! I've incorporated the comments. |
Signed-off-by: Kishore K G <[email protected]>
| * [FEATURE] You can specify "heap ballast" to reduce Go GC Churn #1489 | ||
| * [BUGFIX] HA Tracker no longer always makes a request to Consul/Etcd when a request is not from the active replica #1516 | ||
| * [BUGFIX] Queries are now correctly cancelled by the query-frontend #1508 | ||
| * [BUGFIX] If max_outstanding_requests_per_tenant value is updated to lesser value than the current number of requests in the queue, the excess requests (newest ones) will be dropped to prevent deadlocks. #7188 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move it to master / unreleased? and would be good to add target component like Scheduler: If max_outstanding_requests_per_tenant ~.
What this PR does:
This PR fixes a critical deadlock in the query scheduler that occurs when a tenant's max_outstanding_requests_per_tenant limit is dynamically reduced via runtime configuration.
When MaxOutstandingPerTenant is reduced while a user's FIFORequestQueue is full, the getOrAddQueue method attempts to migrate requests to a smaller queue. Previously, this loop blocked indefinitely when the new queue capacity was reached, causing the scheduler to freeze.
The fix ensures the migration loop breaks when the new queue is full, effectively dropping excess requests instead of blocking.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]