-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[QueryThrottler]Replace polling with topo server watch for config updates #18905
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: main
Are you sure you want to change the base?
[QueryThrottler]Replace polling with topo server watch for config updates #18905
Conversation
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: siddharth16396 <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #18905 +/- ##
==========================================
+ Coverage 69.73% 69.75% +0.01%
==========================================
Files 1608 1607 -1
Lines 214776 214839 +63
==========================================
+ Hits 149781 149863 +82
+ Misses 64995 64976 -19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
|
@siddharth16396 I like this! ❤️ I will review soon. In the meantime, after talking with @shlomi-noach ... (his idea), I think that for v23 we should just remove this log line altogether. The feature is still experimental, and you could leave the log message enabled in your fork if you like. But this way most users (virtually all), who are not using it, will not have this frequent (they might have aggregated logs across hundreds or even thousands of tablets) and likely confusing messages. What do you think? We can open that PR directly against the Thank you! |
|
I have this PR #18904 |
Description
This PR Replaces periodic config polling (60s intervals) with event-driven topology server watches for QueryThrottler configuration updates. Instead of continuously reading config files on a timer, the throttler now subscribes to SrvKeyspace changes and reacts immediately when configs are updated in the topo server.
Changes Made
Removed the polling model:
Added event-driven topo watch:
New proto definitions:
Architecture improvements:
How it works
Benefits achieved:
Related Issue(s)
Checklist