Workqueue: Add generic versions that are properly typed #124263
Conversation
k8s-ci-robot
added
sig/api-machinery
|
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Apr 10 19:46:29 UTC 2024. |
k8s-ci-robot
added
cncf-cla: yes
alvaroaleman
force-pushed
the
typed
branch
3 times, most recently
from
838640f to
cf9fd42
Compare
|
Was this discussed with SIG API machinery? While I would like to see this change, it's highly disruptive to ecosystem controllers which use the client libraries, significantly increasing the cost of upgrading them. |
Adding the generic type you want for your workqueue in the typedef and constructor is |
k8s-ci-robot
added
the
size/L
alvaroaleman
changed the title
|
@alvaroaleman: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
The last similar thing we did was the introduction of typed pointer functions kubernetes/utils#283 As you can see, we created an alternate package. That could allow us to keep shorter function names. Although I'm not sure what an alternative name could be. |
| } | ||
|
|
||
| // DefaultControllerRateLimiter is a no-arg constructor for a default rate limiter for a workqueue. It has | ||
| // both overall and per-item rate limiting. The overall is a token bucket and the per-item is exponential | ||
| // | ||
| // Deprecated: Use DefaultTypedControllerRateLimiter instead. |
There was a problem hiding this comment.
do we also deprecate RateLimiter? Why this symbol, but not many others in this change? (am just curious, no strong opinions. My gut feeling is that many of these are super-wide spread in the ecosystem).
There was a problem hiding this comment.
Just that I missed it. If there are no objections I would like to deprecate all the untyped variants. It will not cause any build failures, but if ppl use linters they will nudge them to start using the typed variants.
| func New() *Type { | ||
| return NewWithConfig(QueueConfig{ | ||
| Name: "", | ||
| }) | ||
| } | ||
|
|
||
| // NewTyped constructs a new work queue (see the package comment). | ||
| func NewTyped[T comparable]() *TypedType[T] { |
There was a problem hiding this comment.
TypedType is stuttering and looks kind of funny. Maybe just Typed?
k8s-ci-robot
added
the
needs-rebase
|
Rebased to incorporate #123347 |
| ShutDown() | ||
| ShutDownWithDrain() | ||
| ShuttingDown() bool | ||
| } | ||
|
|
||
| // Queue is the underlying storage for items. The functions below are always | ||
| // called from the same goroutine. | ||
| type Queue interface { | ||
| type Queue[T comparable] interface { |
There was a problem hiding this comment.
Why does this struct not have the Typed approach? i.e.
type Queue TypedQueue[any]
type TypedQueue[T comparable] interface {There was a problem hiding this comment.
Because this got merged yesterday only and I went by the assumption that it is okay to break the interface, given that it has no users in k/k and is not in any released version: #123347
| } | ||
|
|
||
| // DefaultQueue is a slice based FIFO queue. | ||
| func DefaultQueue() Queue { | ||
| return new(queue) | ||
| func DefaultQueue[T comparable]() Queue[T] { |
There was a problem hiding this comment.
Does this also need the Typed approach?
k8s-ci-robot
removed
the
needs-rebase
This change adds a generic version of the various workqueue types while retaining compatibility for the existing exported symbols and constructors. The generic variants are prefixed with `Typed` and the existing ones are marked as deprecated to nudge people to transition without breaking them.
|
Did another pass. Looks good. From Slack I figure that everybody is on-board with the direction. /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 3fa3ba435076b43d22c4fc0d0f67cb61b7301bca
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, sttts 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
References: kubernetes/kubernetes#124263 kubernetes-sigs/controller-runtime#2799 Signed-off-by: Cem Mergenci <cmergenci@gmail.com>
This change adds a generic version of the various workqueue types while
retaining compatibility for the existing exported symbols and constructors.
The generic variants are prefixed with
Typedand the existing ones aremarked as deprecated to nudge people to transition without breaking
them.
The first commit is the changes in the workqueue itself, the second changes the Customresource Discovery Controller to use a typed queue in order to demonstrate it.
/sig api-machinery
/kind cleanup
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: