-
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
Tablet throttler: recent check diff fix #16001
Tablet throttler: recent check diff fix #16001
Conversation
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 #16001 +/- ##
==========================================
+ Coverage 68.24% 68.26% +0.01%
==========================================
Files 1562 1562
Lines 197171 197185 +14
==========================================
+ Hits 134550 134599 +49
+ Misses 62621 62586 -35 ☔ View full report in Codecov by Sentry. |
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.
Thanks! ❤️
{ | ||
checkResult := throttler.CheckByType(ctx, throttlerapp.VitessName.String(), "", flags, ThrottleCheckSelf) | ||
assert.False(t, checkResult.RecentlyChecked) // "vitess" app does not mark the throttler as recently checked | ||
assert.False(t, throttler.recentlyChecked()) // "vitess" app does not mark the throttler as recently checked |
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 one isn't by app name, is it?
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.
Not this one. This is a general purpose "has anyone been bothering this throttler"
Description
Followup to #15397 and #15398
When the
Primary
throttler probes a replica throttler, the replica'sCheckResult
includes aRecentlyChecked
boolean field.That field was not set correctly, and was
false
in almost all internal vitess probing situations. The stimulation introduced in #15398 thankfully prevents a complete starvation of the throttler. However, withRecentlyChecked
reported asfalse
more than it should, thePrimary
will not renew the heartbeat lease as much as it should, leading to more throttling scenarios.In this PR we ensure to return a
RecentlyChecked = true
value when the throttler has, in fact, been recently checked in the past second-or-so, by any other check.Related Issue(s)
#15397
Found while working/testing #15988
Checklist