Allow more than 1000 concurrent revisions when CC!=0#8799
Allow more than 1000 concurrent revisions when CC!=0#8799knative-prow-robot merged 4 commits intoknative:masterfrom
Conversation
We unfortunately need to set some sort of limit because breaker is backed
by a chan struct{}, but it can and should be much higher than 1000.
Also added some more comments and renamed breakerParams to
revisionBreakerParams since this was super-confusing to me, but this
file needs more cleanup.
knative-prow-robot
added
the
size/M
knative-prow-robot
left a comment
knative-prow-robot
left a comment
There was a problem hiding this comment.
@julz: 0 warnings.
Details
In response to this:
We need to set some sort of limit on the revision throttler because the breaker is backed by a chan struct{}, but it can and should be higher than 1000 concurrent requests.
Also added some more comments and renamed
breakerParamstorevisionBreakerParamssince this was super-confusing to me, but this file could probably do with some more cleanup./assign @markusthoemmes @vagababov
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.
knative-prow-robot
added
area/autoscale
approved
| // to this value. We need to set some value here since the breaker requires | ||
| // an explicit buffer size (it's backed by a chan struct{}), but it can | ||
| // safely be quite large. | ||
| breakerMaxConcurrency = 100000 |
There was a problem hiding this comment.
Any reason this is not just the max buffer length of channels anyway? Or Int32Max or something?
Maybe we should set a constant in the breaker implementation itself and reuse that here. I.e. queue.MaxBreakerCapacity and describe there why it's set to the respective value.
There was a problem hiding this comment.
yeah, can't think of a reason not to, and the constant in queue is a good idea. Turned out this needed some test fixes since they try to create MaxConcurrency trackers, though.
knative-prow-robot
removed
the
approved
markusthoemmes
left a comment
markusthoemmes
left a comment
There was a problem hiding this comment.
/lgtm
/hold
In case @vagababov knows some Go internals that would prevent us from doing this 😅.
knative-prow-robot
added
do-not-merge/hold
| // as MaxConcurrency. This will be very, very slow if MaxConcurrency is very large. | ||
| bmp := revisionBreakerParams.MaxConcurrency | ||
| revisionBreakerParams.MaxConcurrency = 1000 | ||
| defer func() { revisionBreakerParams.MaxConcurrency = bmp }() |
|
lgtm in general :) |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: julz, vagababov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
/unhold |
knative-prow-robot
removed
the
do-not-merge/hold
|
/retest |
|
/lgtm |
|
/retest |
|
The failed one looks not relative /retest |
|
🤷 |
|
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests: |
|
:( /retest |
7 participants
We need to set some sort of limit on the revision throttler because the breaker is backed by a chan struct{}, but it can and should be higher than 1000 concurrent requests.
Also added some more comments and renamed
breakerParamstorevisionBreakerParamssince this was super-confusing to me, but this file could probably do with some more cleanup./assign @markusthoemmes @vagababov