-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Multi-metrics throttler post v21 cleanup: remove unthrottled entry from topo #17283
Multi-metrics throttler post v21 cleanup: remove unthrottled entry from topo #17283
Conversation
…expiration duration Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17283 +/- ##
=======================================
Coverage 67.41% 67.41%
=======================================
Files 1576 1576
Lines 253417 253408 -9
=======================================
- Hits 170846 170840 -6
+ Misses 82571 82568 -3 ☔ View full report in Codecov by Sentry. |
6018f43
to
5368c8b
Compare
5368c8b
to
6018f43
Compare
Signed-off-by: Shlomi Noach <[email protected]>
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.
❤️ This will make it much easier to understand the config.
Description
Multi metrics throttler was introduced in v21 and was backwards compatible with the v20 single-metric throttler. Compatibility included gRPC, command line flags, etc.
Starting v22, we remove single-metric throttler compatibility. This PR is a small extract of #16915, addressing a single issue: when an app is unthrottled (or expired), we now remove its configuration from topo's
Keyspace
record, as opposed to leaving it there with zero values.This helps maintaining the topo record small and tidy. This could not have been done in v21 for compatibility reasons: the throttler maintains an in-memory representation of the config. Up to v20, it would not remove entries from its in-memory represenation even if those entries did not exist in the config. It would only overwrite based on existing config entries. v21 knows better and removes such entries, which is why it's only safe to submit this PR in v22.
Related Issue(s)
#15624
Checklist
Deployment Notes