✨ WithLowPriorityWhenUnchanged: Set Priority for all add methods #3290
Conversation
Making the priority opt a pointer allows to dinstinguish "unset" from "0". While this is irrelevant for the priorityqueue itself, it is required for any wrapper that wants to inject a default priority without overriding one that was explicitly set.
This change makes `WithLowPriorityWhenUnchanged` set the priority for AddAfter, AddRatelimited and AddWithOpts (if not already set) as well.
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman 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
cncf-cla: yes
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
size/L
| addFunc func(item request, q workqueue.TypedRateLimitingInterface[request]) | ||
| type workqueueWithDefaultPriority[request comparable] struct { | ||
| priorityqueue.PriorityQueue[request] | ||
| priority int |
There was a problem hiding this comment.
I'm wondering if we should also make this a pointer
So that we only call AddWithOpts with a priority if it is actually set (and not &0 if unset)
I know that currently for our priority queue implementation it doesn't matter (as far as I can tell), still wondering if it would be better
| @@ -223,7 +226,7 @@ func addToQueueCreate[T client.Object, request comparable](q workqueue.TypedRate | |||
| if evt.IsInInitialList { | |||
| priority = LowPriority | |||
| } | |||
| priorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: priority}, item) | |||
| priorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: &priority}, item) | |||
There was a problem hiding this comment.
Similar here. Wondering if we should just play it safe
var priority *int
if evt.IsInInitialList {
priority = ptr.To(LowPriority)
}
priorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: priority}, item)(same in l.245)
| } else { | ||
| Expect(actualOpts).To(Or( | ||
| Equal(priorityqueue.AddOpts{After: test.after, RateLimited: test.ratelimited}), | ||
| Equal(priorityqueue.AddOpts{After: test.after, RateLimited: test.ratelimited, Priority: ptr.To(0)}), |
There was a problem hiding this comment.
Wondering if this line should be here (if we make the changes I suggested in eventhandler.go)
(same in l.1081)
This change makes
WithLowPriorityWhenUnchangedset the priority forAddAfter, AddRatelimited and AddWithOpts (if not already set) as well.
Fixes #3171
Based on top of #3289 so that needs to merge first
/hold