Raise default REST client QPS/burst when unset#2414
Raise default REST client QPS/burst when unset#2414AryanP123 wants to merge 1 commit intoskupperproject:mainfrom
Conversation
|
Context: With many Listeners/Connectors, readiness lagged because rest.Config had QPS/Burst = 0, so client-go used implicit ~5 QPS / 10 burst and the controller spent a long time client-throttled. Set QPS=100, Burst=200 when both are still unset and RateLimiter is nil Not dynamic. Values apply when clients are built at process start. change env --> restart controller. Testing e2e: |
| if cfg.QPS > 0 && cfg.Burst <= 0 { | ||
| b := int(cfg.QPS * 2) | ||
| if b < 100 { | ||
| b = 100 |
There was a problem hiding this comment.
Not sure if 100 is a good number if, for example, QPS==1, maybe just use what you have on line 49?
| } | ||
| if cfg.Burst > 0 && cfg.QPS <= 0 { | ||
| cfg.QPS = float32(cfg.Burst) / 4 | ||
| if cfg.QPS < 25 { |
There was a problem hiding this comment.
Same thing here. If Burst is set to 4, is it appropriate to set this to 25?
fgiorgetti
left a comment
fgiorgetti
left a comment
There was a problem hiding this comment.
Overall results are really good with the suggested defaults.
Total e2e test time dropped from 16m to 6m locally.
| return | ||
| } | ||
| if cfg.QPS > 0 && cfg.Burst <= 0 { | ||
| b := int(cfg.QPS * 2) |
There was a problem hiding this comment.
what is the reasoning behind this operation and the other one in line 56?
There was a problem hiding this comment.
For when only QPS is set, client-go still needs a burst for the token bucket, so we set it to twice the qps. And then when only burst is set, the same idea but in reverse.
AryanP123
force-pushed
the
controller-kube-client-qps
branch
from
6fbc86e to
45757dd
Compare
3 participants
Fixes #2412