-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[release-0.20] 🌱 Handlers: Default to LowPriorityWhenUnchanged without a wrapper #3179
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[release-0.20] 🌱 Handlers: Default to LowPriorityWhenUnchanged without a wrapper #3179
Conversation
…e interface (kubernetes-sigs#3111) * implement priority queue for handlers Signed-off-by: Troy Connor <troy0820@users.noreply.github.com> * check object before placing in priorityqueue Signed-off-by: Troy Connor <troy0820@users.noreply.github.com> * implement priority queue Signed-off-by: Troy Connor <troy0820@users.noreply.github.com> --------- Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>
This change makes the `TypedFuncs` and `enqueueRequestsFromMapFunc` set a low priority when the object is unchanged by default, as well as extend the test coverage to validate this behavior for `EnqueueRequestForOwner`.
Addresses Stefans comments
The handlers themselves already support this, so there is no need to also do it in the builder.
Signed-off-by: Stefan Büringer buringerst@vmware.com
We already did this for RequestForOwner, but not the typed variation, this change adds that.
e.mapAndEnqueue(ctx, q, evt.Object, reqs) | ||
|
||
var lowPriority bool | ||
if e.objectImplementsClientObject && isPriorityQueue(q) && !isNil(evt.Object) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to also cherry-pick: #3162
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take that back. That would introduce too many changes
Thank you! /lgtm |
LGTM label has been added. Git tree hash: 9158825b3f0e9ff576c6a1c57232a98bd033c60c
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, sbueringer 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 |
This PR cherry-picks all the changes related to #3105 onto release-0.20
/assign @sbueringer