Avoid creating more delete collection workers than items to delete #91544
Conversation
k8s-ci-robot
added
kind/cleanup
k8s-ci-robot
added
area/apiserver
sig/api-machinery
|
/retest |
|
/assign @liggitt |
|
/assign @wojtek-t |
| // DeleteCollectionWorkers (it doesn't make much sense to spawn 16 | ||
| // workers to delete 10 items). | ||
| workersNumber := e.DeleteCollectionWorkers | ||
| if workersNumber < 1 { | ||
| workersNumber = 1 | ||
| } else if workersNumber > len(items) { | ||
| workersNumber = len(items) |
There was a problem hiding this comment.
what happens with zero items/workers?
There was a problem hiding this comment.
it's probably fine, since there isn't anything to delete then
But I would probably feel safer maxing it with 1,
So basically:
max(1, min(len(items), e.DeleteCollectionWorkers)
There was a problem hiding this comment.
Agreed, it will at least clarify the case... I'll make the change
There was a problem hiding this comment.
ptal
I moved the if around to take care of the 0 items case.
Also, do you think it is a good idea to just return up front if there are no items in the collection (I don't know if this can even happen)?
I added a check to just return successfully if there are no items in the collection, instead of going through the WaitGroup and workers code which ultimately won't do anything.
brianpursley
force-pushed
the
DeleteCollectionWorkers
branch
from
e445611
to
88885b6
Compare
|
/retest |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brianpursley, 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
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Was investigating timeout flakes and saw this TODO comment. I don't think this is the cause of the timeout flakes, but it does seem like a good idea to limit the number of workers to the number of items being deleted.
Practically speaking this will have a limited effect since the default DeleteCollectionWorkers appears to be 1, but if you set it higher, it will avoid creating extra unnecessary workers.
Which issue(s) this PR fixes:
I could not find an issue for this, but there was a TODO comment in the code.
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: