Implement ItemBucketRateLimiter #88261
Conversation
k8s-ci-robot
added
release-note-none
|
/assign @wojtek-t |
k8s-ci-robot
added
sig/api-machinery
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mborsz, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest |
|
@fedebongio: GitHub didn't allow me to request PR reviews from the following users: jktomer. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
|
||
| var _ RateLimiter = &ItemBucketRateLimiter{} | ||
|
|
||
| // NewItemBucketRateLimiter creates new ItemBucketRateLimiter instance. |
There was a problem hiding this comment.
What is the rationale for using this kind of rate limit for erroring items?
There was a problem hiding this comment.
My intent was not to use this for erroring items, rather for success case.
I know that it might be slightly misleading. Do you have idea how to improve this?
I was thinking about:
- adding a comment
- maybe removing this from this file (the fact that it implements RateLimiter suggests that this can be used for error case)
Any other idea? WDYT?
There was a problem hiding this comment.
I think that adding a comment should be enough - just descibe what are the usecases that we expect it to be used at (it's roughly Daniel`s comment from above: https://github.com/kubernetes/kubernetes/pull/88261/files/6846a0a6b62725a5888d66e2789f2af4fb172045#r380980162 )
| @@ -62,6 +62,54 @@ func (r *BucketRateLimiter) NumRequeues(item interface{}) int { | |||
| func (r *BucketRateLimiter) Forget(item interface{}) { | |||
| } | |||
|
|
|||
| // ItemBucketRateLimiter implements a workqueue ratelimiter API using standard rate.Limiter. | |||
| // Each key is using a separate limiter. | |||
There was a problem hiding this comment.
I think it'd be good to explain under what conditions it makes sense to use this.
| burst int | ||
|
|
||
| limitersLock sync.Mutex | ||
| limiters map[interface{}]*rate.Limiter |
There was a problem hiding this comment.
Isn't this pretty heavyweight if there's a lot of keys?
There was a problem hiding this comment.
hm, I guess this limiter isn't as bad as I thought.
What type of PR is this?
/kind feature
What this PR does / why we need it:
This implements a simple workqueue rate limiter that is using rate.Limiter for each key separately.
It is intended to be used in #88161.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: