-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[QueryThrottler]Replace polling with topo server watch for config updates #18979
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 #18979
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]>
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]>
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
This reverts commit 8de07c7. 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]>
|
The description looks good to me! I didn't get to this before my holiday vacation, but I promise to do it the week that I'm back. Thank you for the contribution! I apologize for the delay. |
mattlord
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.
LGTM! Just a number of minor comments/questions/suggestions. Let me know what you think
| srvTopoServer srvtopo.Server | ||
|
|
||
| mu sync.RWMutex | ||
| watchStarted atomic.Bool |
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.
Why not use a sync.Once instance?
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.
sync.Once cannot be reset. Shutdown() resets watchStarted to false to allow potential restarts (useful for testing and defensive design).
Though we're certain QueryThrottler is never restarted after shutdown and maybe we could switch to sync.Once, but the current approach provides more flexibility for tests and future use cases and hence we used atomic.Bool
| // isConfigUpdateRequired checks if the new config is different from the old config. | ||
| // This only checks for enabled, strategy name, and dry run because the strategy itself will update the strategy-specific config | ||
| // during runtime by having a separate watcher similar to the one used in QueryThrottler. | ||
| func isConfigUpdateRequired(oldCfg, newCfg Config) bool { | ||
| if oldCfg.Enabled != newCfg.Enabled { | ||
| return true | ||
| } | ||
|
|
||
| if oldCfg.StrategyName != newCfg.StrategyName { | ||
| return true | ||
| } | ||
|
|
||
| if oldCfg.DryRun != newCfg.DryRun { | ||
| return true | ||
| } | ||
|
|
||
| return false |
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.
Any reason not to use Equal instead? It's another reason to use the proto types directly as we can then use proto.Equal()
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 reason for not using "Equal" was:
The Config proto contains both:
- QueryThrottler-level fields: enabled, strategy, dry_run
- Strategy-specific configs: nested configs for TabletThrottler, custom strategies, etc.
isConfigUpdateRequired intentionally compares only the QueryThrottler-level fields because:
- Separation of concerns: QueryThrottler treats strategy internals as opaque. It only cares whether throttling is enabled, which strategy type to use, and whether dry-run is on.
- Strategy self-management: Each strategy instance watches and handles its own configuration updates independently. When TabletThrottler's internal thresholds change, it reloads them itself - QueryThrottler doesn't need to recreate the strategy.
- Avoiding unnecessary churn: Using proto.Equal() would trigger a full strategy swap whenever any nested field changes, even though the existing strategy instance can handle those updates internally.
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
Signed-off-by: siddharth16396 <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18979 +/- ##
==========================================
+ Coverage 69.87% 69.89% +0.01%
==========================================
Files 1613 1611 -2
Lines 216002 216058 +56
==========================================
+ Hits 150939 151013 +74
+ Misses 65063 65045 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR is clone of #18905 which i messed up pretty badly while rebasing for DCO commits 😢
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