-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: scheduler (14/): add more scheduler logic #427
Conversation
// Done marks a ClusterResourcePlacementKey as done. | ||
Done(crpKey ClusterResourcePlacementKey) | ||
// Forget untracks a ClusterResourcePlacementKey from rate limiter(s) (if any) set up with the queue. | ||
Forget(crpKey ClusterResourcePlacementKey) |
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.
What's the difference between these two?
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.
Hi Ryan! This is the same as a regular rate limiter queue, where Done
marks a key as no longer in processing and Forget
is to untrack the key from the rate limiter. They cannot be merged into one as there are cases where a key must be retried with approval from the rate limiter.
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.
Hi Ryan! I will merge this PR first; if there's any concern over Done() and Forget(), please let me know.
LabelSelector: labels.SelectorFromSet(labels.Set{fleetv1beta1.CRPTrackingLabel: crp.Name}), | ||
} | ||
// TO-DO (chenyu1): this is a very expensive op; explore options for optimization. | ||
if err := s.uncachedReader.List(ctx, bindingList, listOptions); err != nil { |
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.
is there a delete all option?
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.
The deleteAllOf func has a bug and we cannot make use of it. :(
LabelSelector: labels.SelectorFromSet(labels.Set{fleetv1beta1.CRPTrackingLabel: crp.Name}), | ||
} | ||
// TO-DO (chenyu1): this is a very expensive op; explore options for optimization. | ||
if err := s.uncachedReader.List(ctx, bindingList, listOptions); err != nil { |
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.
The deleteAllOf func has a bug and we cannot make use of it. :(
af1f0ae
to
8630d04
Compare
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.
LGTM :)
Description of your changes
This PR is part of the PRs that implement the Fleet workload scheduling.
It features more logic for the scheduler.
I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
To control the size of the PR, certain unit tests are not checked in; they will be sent in a separate PR.