-
-
Notifications
You must be signed in to change notification settings - Fork 608
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
ratelimits: Implement batched Spends and Refunds #7143
Conversation
04a05bd
to
6b70047
Compare
6b70047
to
c96c473
Compare
996f652
to
e1fe8a4
Compare
e1fe8a4
to
6f4bd2a
Compare
731b8a1
to
f8c6528
Compare
f41cd06
to
14920cd
Compare
14920cd
to
4e42b79
Compare
0e5b7d5
to
7d2d757
Compare
fddb710
to
32e393a
Compare
03136d6
to
798a61f
Compare
798a61f
to
0adf3fa
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.
Partial review but I wanted to get something across the table before the end of the (east coast) day. Will keep reviewing.
62ae5a4
to
6f6ec22
Compare
941293e
to
7fd825b
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.
Looks great!
|
||
tats := make(map[string]time.Time, len(bucketKeys)) | ||
for i, result := range results { | ||
tatNano, err := result.(*redis.StringCmd).Int64() |
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.
Is each result documented to definitely be of type *redis.StringCmd
? If not, this should use the , ok
type assertion and return error if it's the wrong type.
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.
Correct. Specifically, each executed Pipeline command is documented to return a specific result type:
A few lines above we queue multiple Get()
commands:
pipeline := r.client.Pipeline()
for _, bucketKey := range bucketKeys {
pipeline.Get(ctx, bucketKey)
}
The function signature of the Get()
command:
func (redis.StringCmdable).Get(ctx context.Context, key string) *redis.StringCmd
|
||
domains = DomainsForRateLimiting([]string{"www.example.com", "example.com", "www.example.co.uk"}) | ||
test.AssertDeepEquals(t, domains, []string{"example.co.uk", "example.com"}) | ||
|
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.
domains = DomainsForRateLimiting([]string{"example.com", "example.com"}) | |
test.AssertDeepEquals(t, domains, []string{"example.com"}) | |
The function description states that it also de-duplicates the output domains.
Part of #5545
Ideally
ratelimits/bucket.go
should be renamedratelimits/transaction.go
but that will make the diff harder to review. We can do the rename in a follow-up.