ratelimits: Refactor Reset() to accept a batch of Transactions #8503
Conversation
beautifulentropy
force-pushed
the
make-resets-take-transactions
branch
from
c9297b0 to
f4ec6cd
Compare
beautifulentropy
changed the title
|
I am satisfied that all keys will be deleted even if some are missing. |
| func (r *RedisSource) Delete(ctx context.Context, bucketKey string) error { | ||
| // BatchDelete deletes the TATs at the specified bucketKeys ('name:id'). A nil | ||
| // return value does not indicate that the bucketKeys existed. | ||
| func (r *RedisSource) BatchDelete(ctx context.Context, bucketKeys []string) error { |
There was a problem hiding this comment.
Can't BatchDelete and Delete just be a single function with variadic args?
func (r *RedisSource) Delete(ctx context.Context, bucketKeys ...string) error {| }, newTestTransactionBuilder(t), clk, randIP.String() | ||
| } | ||
|
|
||
| func mustReset(t *testing.T, l *Limiter, ctx context.Context, limit *Limit, bucketKey string) { |
There was a problem hiding this comment.
This is a nit, but: I think we tend to use must to mean "will panic if a precondition fails", rather than "will fail a test assertion". I don't think I see any other mustFoo functions in the codebase that take a testing.T as an argument; meanwhile we do have mustFoo test functions that use our must.Do helper.
Maybe this is totally fine? I think the meaning and usage is mostly clear. But it just blurs the line between this helper being a test function that readers need to care about, versus it being boilerplate that we're clearly stating can never fail.
| // - allow-only: when neither check nor spend are true, the transaction will | ||
| // be considered "allowed" regardless of the bucket's capacity. This is | ||
| // useful for limits that are disabled. | ||
| // - reset: when reset is true, the bucket will be reset to full capacity. |
There was a problem hiding this comment.
nit: name this "reset-only", and put it above "allow-only".
Fixes #8305