RateLimiter: change CanAccept() to TryAccept() #17694
Conversation
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
1 similar comment
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
hongchaodeng
force-pushed
the
ratelimit
branch
from
3f762cb to
13d152a
Compare
|
Labelling this PR as size/S |
k8s-github-robot
added
the
size/S
|
don't feel super strongly, but I think I'd prefer just updating the documentation on the |
|
@liggitt I feel |
|
If the plan is to add a function that checks with no side-effect, then I agree that function should be called |
| @@ -47,7 +48,7 @@ func NewFakeRateLimiter() RateLimiter { | |||
| return &fakeRateLimiter{} | |||
| } | |||
|
|
|||
| func (t *tickRateLimiter) CanAccept() bool { | |||
| func (t *tickRateLimiter) TryAccept() bool { | |||
There was a problem hiding this comment.
Godoc please while you are here.
There was a problem hiding this comment.
This is private implementation.
The docs should be covered in public interface (i.e. RateLimiter), right?
There was a problem hiding this comment.
ah, sorry, I agree. thanks!
There was a problem hiding this comment.
Great. Thanks!
|
nit on godoc for public functions. otherwise lgtm. |
|
my mistake on the godoc comment. lgtm. |
derekwaynecarr
added
the
lgtm
|
@k8s-bot ok to test |
|
Continuous integration appears to have missed, closing and re-opening to trigger it |
|
GCE e2e test build/test passed for commit 13d152a. |
|
The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label? |
|
@derekwaynecarr We are working on the |
|
I'm curious how you anticipate a non-token-acquiring |
|
@liggitt Oh. We want to expose the metrics. Like is this ratelimiter limiting the rate, what is the current filling rate, etc.. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit 13d152a. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit 13d152a. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
The
CanAcceptinterface description is not accurate. The actual behavior is taking a token instead of just testing without side-effect. And all the users of the interface relies on the taking behavior, not testing.This commit gives it a better name and also a more accurate description.
/cc @brendandburns @liggitt