-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
kvserver: the timeout of queued items should consider the rates of al… #83667
Conversation
0ab4037
to
24649ef
Compare
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.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @shralex)
pkg/kv/kvserver/queue_test.go
line 996 at r1 (raw file):
queueGuaranteedProcessingTimeBudget.Override(ctx, &st.SV, tc.guaranteedProcessingTime) recoverySnapshotRate.Override(ctx, &st.SV, tc.rateLimit) tf := makeRateLimitedTimeoutFunc(recoverySnapshotRate, rebalanceSnapshotRate)
Should we expand this test to exercise the case where the second rate limit is lower than the first?
pkg/kv/kvserver/queue_test.go
line 996 at r1 (raw file):
queueGuaranteedProcessingTimeBudget.Override(ctx, &st.SV, tc.guaranteedProcessingTime) recoverySnapshotRate.Override(ctx, &st.SV, tc.rateLimit) tf := makeRateLimitedTimeoutFunc(recoverySnapshotRate, rebalanceSnapshotRate)
We're not overriding the default of rebalanceSnapshotRate
, so there's a potential that a change to the default value for rebalanceSnapshotRate
will break the test. Let's override the setting with either a constant value or (even better) a value that varies between test cases so we can address the previous comment.
4dec714
to
a2f7bbd
Compare
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @shralex)
-- commits
line 2 at r2:
This commit message appears to have been doubled.
…l item types in the queue When the kv.snapshot_recovery.max_rate and kv.snapshot_rebalance.max_rate settings are given different values, if the recovery rate is high and the rebalance rate is low, recovery snapshots can have a lower timeout than the expected duration of a single rebalance snapshot. This means that any steady rebalance load can starve recovery snapshots. To mitigate the issue, this PR sets the timeout for a snapshot based on min(kv.snapshot_recovery.max_rate, kv.snapshot_rebalance.max_rate). Release note: None
a2f7bbd
to
ead518f
Compare
bors r+ |
Build succeeded: |
@shralex do we want to backport this to v21.2 and v22.1? |
blathers backport 21.2 |
blathers backport 22.1 |
…l item types in the queue
When the kv.snapshot_recovery.max_rate and kv.snapshot_rebalance.max_rate
settings are given different values, if the recovery rate is high and the
rebalance rate is low, recovery snapshots can have a lower timeout than the
expected duration of a single rebalance snapshot. This means that any steady
rebalance load can starve recovery snapshots. To mitigate the issue, this PR
sets the timeout for a snapshot based on min(kv.snapshot_recovery.max_rate,
kv.snapshot_rebalance.max_rate).
Release note: None