-
Notifications
You must be signed in to change notification settings - Fork 917
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
Optimize deprioritized policy preemption logic #4555
Optimize deprioritized policy preemption logic #4555
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #4555 +/- ##
==========================================
- Coverage 51.86% 51.40% -0.47%
==========================================
Files 243 250 +7
Lines 24183 24979 +796
==========================================
+ Hits 12543 12840 +297
- Misses 10959 11433 +474
- Partials 681 706 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ping @RainbowMango @chaosi-zju |
the code in this PR looks good, thank you for your effort! |
Seems this PR trying to fix a bug? |
More like optimization cause multiple preemptions do not affect its functionality. |
845da7d
to
9dc6be7
Compare
@chaosi-zju @RainbowMango |
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.
Thanks~
This is a great optimization.
LGTM.
ping @RainbowMango |
/assign |
By the way, have you ever considered the PriorityQueue which looks like more simple than the tree. |
Nice reminder. |
9dc6be7
to
efd56ec
Compare
Their performance is almost the same(both |
Seems the related testing is failing:
https://github.com/karmada-io/karmada/actions/runs/8078521320/job/22071318982?pr=4555 |
efd56ec
to
acad217
Compare
acad217
to
7bda80d
Compare
Use the priorityequeue to sort the listed policies to avoid multiple preemption. Signed-off-by: whitewindmills <[email protected]>
7bda80d
to
e88797b
Compare
@RainbowMango Ready to review again. |
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
/approve
Thanks.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango 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 |
What type of PR is this?
/kind feature
/kind cleanup
What this PR does / why we need it:
Using the natural ordering properties of red-black trees to sort the listed policies to ensure the higher priority (Cluster)PropagationPolicy being processed first to avoid possible multiple preemption.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: