tenantcostclient: smooth over debt payback#71149
tenantcostclient: smooth over debt payback#71149craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
andy-kimball
left a comment
There was a problem hiding this comment.
I'm surprised you don't have any data-driven tests here. Are those not useful for this new case?
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @RaduBerinde)
pkg/ccl/multitenantccl/tenantcostclient/token_bucket.go, line 48 at r1 (raw file):
// This rate is recalculated every time we incur debt. So in essence, every time // we incur debt, we put it together with all existing debt and plan to pay it // within time T (we can think of this as "refinancing" the existing debt).
😂 We should charge an interest rate.
pkg/ccl/multitenantccl/tenantcostclient/token_bucket.go, line 172 at r1 (raw file):
// increasing or decreasing them. // // A positive amount indicates
NIT: This comment needs fixing up.
74c7a5c to
da80b3f
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
The datadriven tests are very hard to set up. I added one now - it took me ~1hr to get it right :)
TestTokenBucketTryToFulfill was much more useful.
Reviewable status:
andy-kimball
left a comment
There was a problem hiding this comment.
Reviewable status:
RaduBerinde
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build failed: |
|
The new datadriven test flaked (even though I tested it with both |
Prior to this commit, the tenant's local token bucket stalls all traffic whenever it incurs debt. This is a problem because some debt comes in discrete "packets" (CPU usage is accounted for once a second), which can lead to large occasional latency spikes. This change improves the debt repayment mechanism: instead of paying the debt as soon as possible, we aim to pay the debt within 2 seconds of when the debt is incurred and earmark part of the rate for debt repayment; the remaining rate can be used by requests in the mean time. The comment in `token_bucket.go` goes over more details. Release note: None Release justification: Necessary fix for the distributed rate limiting functionality, which is vital for the upcoming Serverless MVP release. It allows CRDB to throttle clusters that have run out of free or paid request units (which measure CPU and I/O usage). This functionality is only enabled in multi-tenant scenarios and should have no impact on our dedicated customers.
da80b3f to
e3417f2
Compare
|
Ah, the problem was that I wasn't rebased on top of the latest cost model changes. Fixed, will try again after CI runs. |
|
bors r+ |
|
Build succeeded: |
Prior to this commit, the tenant's local token bucket stalls all
traffic whenever it incurs debt. This is a problem because some debt
comes in discrete "packets" (CPU usage is accounted for once a
second), which can lead to large occasional latency spikes.
This change improves the debt repayment mechanism: instead of paying
the debt as soon as possible, we aim to pay the debt within 2 seconds
of when the debt is incurred and earmark part of the rate for debt
repayment; the remaining rate can be used by requests in the mean
time. The comment in
token_bucket.gogoes over more details.Release note: None
Release justification: Necessary fix for the distributed rate limiting
functionality, which is vital for the upcoming Serverless MVP release.
It allows CRDB to throttle clusters that have run out of free or paid
request units (which measure CPU and I/O usage). This functionality is
only enabled in multi-tenant scenarios and should have no impact on
our dedicated customers.
Plots for latency data from the debt test
Before:
After: