release-21.1: tenantrate: switch to a single token bucket #65347
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
ajwerner
approved these changes
May 24, 2021
complete! 1 of 0 LGTMs obtained
This change adds a "test" facility which takes the description of a uniform workload (read percentage, read size, write size) and prints out an estimation of the sustained IOPS and burst IO. This will allow a better understanding of how changes to the settings or the mechanism translate into IOPS changes. Release note: None
The `Merge` method was clunky and required type switched. Instead, the API is changed to take a closure. Amazingly none of these closures escape! Release note: None
This commit extracts the token bucket accounting logic into a separate, public type. The same code will be used by the tenantrate limiter. Release note: None
Tenant rate limiting currently uses four separate token buckets, each with its own rate and burst limits. In this commit we switch to a single shared token bucket (see cockroachdb#55114 for motivation). We use a "cost model" to map read and write requests to "KV Compute Units". Requests have a base cost plus a per-byte cost. The details are documented in settings.go. The values were chosen based on experiments ran by Nathan: https://docs.google.com/spreadsheets/d/1PPlIcKnusOqWtBoOZVd9xBEMPe5Ss1FgrJlYFgpQZaM/edit#gid=735409177 The rate was chosen so that it maps to 20% of 1 KV vCPU. This keeps the rate limit on small requests roughly the same as before (especially on mixed workloads). The largest departure from the previous limits is that we allow much more read bytes (the per-byte cost of reads is small in the cost model). If we were to keep close to the previous limits, the value of kv.tenant_rate_limiter.read_cost_per_megabyte would be 200 instead of 10. Perhaps we want to be more conservative here and make this value somewhere in-between? Fixes cockroachdb#55114. Release note: None
The way the test interacts with the configuration (through cluster settings) is flaky - when we modify multiple settings and the head of the queue gets notified, it might or might not observe the intermediary state. This commit cleans things up: the test now directly updates the config rather than going through the cluster settings. Fixes cockroachdb#63621. Release note: None
RaduBerinde
force-pushed
the
backport21.1-62814-63227-62783
branch
from
7b08132 to
f6b620d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport:
Please see individual PRs for details.
/cc @cockroachdb/release