Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
📖 Add a design for a priority queue #3013
📖 Add a design for a priority queue #3013
Changes from all commits
bd6eede
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Will the default controller be modified to make use of this new queue (if used), or will it rely on using a custom controller implementation?
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.
Yes it will be updated to make use of it - the only thing it needs is to re-use the priority though. Once we make it the default, I would also like to add a Priority parameter to the
reconcile.Result
- But really only once its enabled by default.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.
Maybe also to the Request?
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.
Ah, interesting thought. Is the idea because you want to be able to set the priority in a handler?
The way this currently works is that the workqueue doesn't have any understanding of the request object (and we should keep it that way IMHO). We could probably provide a thin wrapper for it that will use the priority from the request if the
AddWithOptions
call doesn't have one yet and inject that in the builder if its typed toreconcile.Request
?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 was thinking that the priority could also be useful information for the Reconcile func. But maybe it's a bad idea not sure 😀
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.
Ah. Because then it can use that as an input when returning a priority?
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.
That's one use case. Another one would be to reconcile Requests of different priorities differently :)
Maybe it makes sense in some cases to pass down the priority (if some other components are involved in reconciliation).
Maybe a controller would act differently if it can infer based on the priority if this is just a periodic resync (or something similar) vs an actual change. Or in general prioritize Requests with higher priority higher (if it has to schedule some "tasks" in other systems)
But I'm really not sure if this is a good idea or opening pandora's box :)
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.
Let's follow-up on this with more use cases later on, if needed