Skip to content
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

descheduler: optimize sortfn implementation #1711

Closed
wants to merge 6 commits into from

Conversation

baowj-678
Copy link
Member

Ⅰ. Describe what this PR does

optimize sortfn implementation.

Ⅱ. Does this pull request fix one issue?

fixes #1644

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in make test

@koordinator-bot koordinator-bot bot requested review from eahydra and FillZpp October 17, 2023 03:09
@koordinator-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign eahydra after the PR has been reviewed.
You can assign the PR to them by writing /assign @eahydra in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (8c19de1) 65.91% compared to head (9c9b46f) 65.96%.
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1711      +/-   ##
==========================================
+ Coverage   65.91%   65.96%   +0.05%     
==========================================
  Files         385      386       +1     
  Lines       41639    41918     +279     
==========================================
+ Hits        27447    27652     +205     
- Misses      12155    12224      +69     
- Partials     2037     2042       +5     
Flag Coverage Δ
unittests 65.96% <67.92%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...scheduler/controllers/migration/arbitrator/sort.go 81.35% <89.47%> (-2.81%) ⬇️
...ler/controllers/migration/arbitrator/arbitrator.go 60.34% <13.33%> (-5.47%) ⬇️

... and 17 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@baowj-678 baowj-678 changed the title optimize sortfn implementation koord-descheduler: optimize sortfn implementation Oct 17, 2023
// SortFn stably sorts PodMigrationJobs slice based on a certain strategy. Users
// can implement different SortFn according to their needs.
type SortFn func(jobs []*v1alpha1.PodMigrationJob, podOfJob map[*v1alpha1.PodMigrationJob]*corev1.Pod) []*v1alpha1.PodMigrationJob
type SortFn func(jobs []*v1alpha1.PodMigrationJob, podOfJob map[*v1alpha1.PodMigrationJob]*corev1.Pod) CompareFn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么一个SortFn 的函数的返回值是一个CompareFn? SortFn对输入的对象排序,按理说是不需要返回值的,更不要说返回一个CompareFn。
是不是可以理解为之前的SortFn都应该转为CompareFn?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是的,之前的sortfn会输出每个job的排序优先级的map,然后返回的comparefn会依据该map进行排序,这样sort函数就只用调用一次

return job.Status.Phase == v1alpha1.PodMigrationJobRunning ||
(job.Status.Phase == v1alpha1.PodMigrationJobPending && f.checkJobPassedArbitration(job.UID))
}),
SortJobsByPod([]sorter.CompareFn{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个实现和之前相比较,没有必要的。就算是需要,我想也可以再构造一个sorter传入也OK?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

嗯嗯,我改成从New中以参数传入

@eahydra eahydra changed the title koord-descheduler: optimize sortfn implementation descheduler: optimize sortfn implementation Oct 19, 2023
@ZiMengSheng
Copy link
Contributor

/milestone v1.4

@koordinator-bot koordinator-bot bot added this to the v1.4 milestone Jan 2, 2024
@eahydra eahydra closed this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[proposal] optimize Arbitration SortFn
3 participants