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 {There was a problem hiding this comment.
Actually, I’m just going to remove Delete(). I had a caller for it originally, but I removed that and forgot to drop the implementation. As for switching from []string to a variadic, it doesn’t feel consistent unless BatchGet() changes too, and modifying the Source interface that much doesn’t seem worthwhile unless there’s a compelling reason I’m missing??
There was a problem hiding this comment.
Although the Limiter exposes both Spend and BatchSpend, it's unclear what benefit we gain by forcing their callers to track whether they're currently handling a single transaction or a collection of transactions. Those functions' return types are the same, so being combined into a single variadic function would have no impact on any of the calling code. The same is true of Refund and BatchRefund.
If there's no real benefit to the Limiter exposing Batch and non-Batch methods, then it seems like there's likely no real benefit to the Source exposing Batch and non-Batch methods either. And indeed, after this PR, the only non-Batch method will be .Get. There's only one caller of .Get(), which is limiter.Check. There are no callers of limiter.Check.
So yes, I agree that there's not much call to be doing those simplifications in this PR. But I think we should file a ticket for follow-up to remove unnecessary duplication between batch and non-batch methods on the source and limiter types.
| 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.
Although the Limiter exposes both Spend and BatchSpend, it's unclear what benefit we gain by forcing their callers to track whether they're currently handling a single transaction or a collection of transactions. Those functions' return types are the same, so being combined into a single variadic function would have no impact on any of the calling code. The same is true of Refund and BatchRefund.
If there's no real benefit to the Limiter exposing Batch and non-Batch methods, then it seems like there's likely no real benefit to the Source exposing Batch and non-Batch methods either. And indeed, after this PR, the only non-Batch method will be .Get. There's only one caller of .Get(), which is limiter.Check. There are no callers of limiter.Check.
So yes, I agree that there's not much call to be doing those simplifications in this PR. But I think we should file a ticket for follow-up to remove unnecessary duplication between batch and non-batch methods on the source and limiter types.
Fixes #8305