All clients under ClientSet share one RateLimiter. #24166
Conversation
k8s-github-robot
added
size/M
| @@ -87,6 +88,9 @@ type Config struct { | |||
|
|
|||
| // Maximum burst for throttle | |||
| Burst int | |||
|
|
|||
| // Rate limiter for limiting connections to the master from this client. If present overwrites QPS/Burst | |||
There was a problem hiding this comment.
What prevents replacing QPS/Burst?
There was a problem hiding this comment.
Nothing hard - I'm just following the 'add, don't remove' principle in case it was used somewhere in a serialized form. I can remove them if you believe it's safe.
|
GCE e2e build/test failed for commit 1138a8046449f3fac643d3d3f696b476bf239a58. Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake. |
|
If this think looks valid I'll fix tests tomorrow. |
| if c.RateLimiter == nil && c.QPS > 0 { | ||
| c.RateLimiter = flowcontrol.NewTokenBucketRateLimiter(c.QPS, c.Burst) | ||
| } | ||
| var clientset Clientset |
There was a problem hiding this comment.
the indentation in lines you added look strange - please fix
|
GCE e2e build/test passed for commit e2727f31dbd7b4df413463a1c53fdc1185e613ad. |
| if c.RateLimiter == nil && c.QPS > 0 { | ||
| c.RateLimiter = flowcontrol.NewTokenBucketRateLimiter(c.QPS, c.Burst) | ||
| } | ||
| var clientset Clientset |
There was a problem hiding this comment.
It seems we are using "\t" in all other places in this file. Can you please change it to it here too?
gmarek
force-pushed
the
client
branch
2 times, most recently
from
31ed3f8 to
a95bbd9
Compare
|
GCE e2e build/test passed for commit 31ed3f8c7528b99cae55b2d060be0fc09a6eee84. |
|
GCE e2e build/test passed for commit a95bbd9657d49a319e49859f8fe0117b7ce76f85. |
| if c.RateLimiter == nil && c.QPS > 0 { | ||
| c.RateLimiter = flowcontrol.NewTokenBucketRateLimiter(c.QPS, c.Burst) | ||
| } | ||
| var clientset Clientset |
|
GCE e2e build/test failed for commit 78ea2b80f7cd216504d292f335a80606caa3c469. Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake. |
|
Applied your comments. @krousey PTAL |
|
GCE e2e build/test failed for commit 45ad00f24e6eb29ae1674d8d0638e8ca74eedd92. Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake. |
|
@gmarek I would think so. This is definitely going to affect ALL controllers. They will now share a single rate limiter. |
|
GCE e2e build/test passed for commit 45ad00f24e6eb29ae1674d8d0638e8ca74eedd92. |
gmarek
changed the title
gmarek
added
release-note
|
@krousey - Renamed PR into a release-note friendly form. PTAL |
krousey
added
the
lgtm
|
GCE e2e build/test passed for commit b76bed0. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit b76bed0. |
|
Automatic merge from submit-queue |
Currently we create a rate limiter for each client in client set. It makes the reasoning about rate limiting behavior much harder. This PR changes this behavior and now all clients in the set share single rate limiter. Ref. #24157
cc @lavalamp @wojtek-t