-
Notifications
You must be signed in to change notification settings - Fork 885
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
Implement calc group score for duplicate to assign replicas evenly #5622
Implement calc group score for duplicate to assign replicas evenly #5622
Conversation
5d9742c
to
59be12b
Compare
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 #5622 +/- ##
==========================================
- Coverage 41.66% 41.64% -0.03%
==========================================
Files 653 653
Lines 55518 55533 +15
==========================================
- Hits 23132 23127 -5
- Misses 30885 30908 +23
+ Partials 1501 1498 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
59be12b
to
e3aafcc
Compare
e3aafcc
to
59be12b
Compare
59be12b
to
36746e4
Compare
wait for #5621 |
i will resolve the conflicts. |
07f6f44
to
df53e4d
Compare
now, i have do something for this PR. i need you guys on resolving the merge conflict. @whitewindmills @RainbowMango |
/unhold |
I can't figure out the intention of this PR.
|
df53e4d
to
7723a18
Compare
I've resolved the conflicts, so we can consider merging now. Additionally, I'll clarify the purpose of this PR: As shown in the code, PR #5621 actually represents the GroupScore algorithm when the replica allocation strategy is set to Divided. In contrast, this PR (#5622) provides the GroupScore algorithm for the Duplicate replica allocation strategy. It stands to reason that with Duplicate, the GroupScore algorithm should differ due to the distinct behavior of replica allocation. The GroupScore algorithm I designed works as follows: in a Group, the more clusters that meet the Replica requirement for available copies, the higher the Group Score. If the number of clusters meeting the Replica requirement is the same, then the Group with a higher average cluster score will have a higher Group Score. @whitewindmills @RainbowMango |
func (info *GroupClustersInfo) calcGroupScore( | ||
clusters []ClusterDetailInfo, | ||
rbSpec *workv1alpha2.ResourceBindingSpec, | ||
minGroups int) int64 { | ||
if checkIfDuplicate(rbSpec) { |
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.
we don't need such a redundant function checkIfDuplicate
, use rbSpec.Placement.ReplicaSchedulingType() == policyv1alpha1.ReplicaSchedulingTypeDuplicated
instead.
|
||
// Here is an example, the rbSpec.Replicas == 50. | ||
|
||
// There is the Group 1, it has three clusters as follows. |
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.
five clusters ...
|
||
// There is another example, the rbSpec.Replicas == 50. | ||
|
||
// There is the Group 1, it has three clusters as follows. |
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.
ditto
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.
some nits, LGTM otherwise.
a7099b3
to
9ab1a33
Compare
I've solved these problems. @whitewindmills |
/approve |
Signed-off-by: ipsum-0320 <trueman.0320@zju.edu.cn> feat: implement calc group score for duplicate to assign replicas evenly Signed-off-by: ipsum-0320 <trueman.0320@zju.edu.cn> feat: calc group score for duplicate to assign replicas evenly Signed-off-by: ipsum-0320 <trueman.0320@zju.edu.cn>
9ab1a33
to
9e349e6
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango, whitewindmills 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 |
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
What type of PR is this?
/kind feature
What this PR does / why we need it:
Optimize the score calculation method to better measure the pros and cons of the group. This PR implements calc group score for duplicate to assign replicas evenly.
Which issue(s) this PR fixes:
Part of #5159
Special notes for your reviewer:
@whitewindmills
Does this PR introduce a user-facing change?: