-
Notifications
You must be signed in to change notification settings - Fork 727
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
scheduler: allow balance-region-scheduler create multiple operators #4008
Conversation
Signed-off-by: bufferflies <[email protected]>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
… feature/schedulerOps
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #4008 +/- ##
==========================================
- Coverage 74.70% 74.67% -0.03%
==========================================
Files 260 260
Lines 26625 26638 +13
==========================================
+ Hits 19889 19893 +4
- Misses 4950 4956 +6
- Partials 1786 1789 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
… feature/schedulerOps
Signed-off-by: bufferflies <[email protected]>
Signed-off-by: bufferflies <[email protected]>
@@ -987,6 +987,7 @@ func (s *testOperatorControllerSuite) TestStoreOverloaded(c *C) { | |||
opt.SetAllStoresLimit(storelimit.RemovePeer, 600) | |||
time.Sleep(time.Second) | |||
for i := 0; i < 10; i++ { | |||
|
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.
remove this line
@@ -1024,10 +1025,10 @@ func (s *testOperatorControllerSuite) TestStoreOverloadedWithReplace(c *C) { | |||
c.Assert(oc.AddOperator(op2), IsTrue) | |||
op3 := newTestOperator(1, tc.GetRegion(2).GetRegionEpoch(), operator.OpRegion, operator.AddPeer{ToStore: 1, PeerID: 3}) | |||
c.Assert(oc.AddOperator(op3), IsFalse) | |||
c.Assert(lb.Schedule(tc), IsNil) | |||
c.Assert(len(lb.Schedule(tc)), Equals, 0) |
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.
HasLen
@@ -51,6 +58,7 @@ func init() { | |||
if err := decoder(conf); err != nil { | |||
return nil, err | |||
} | |||
log.Info("conf", zap.Any("config", conf)) |
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.
prefer to make it more clear, like balance region config?
@bufferflies: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@@ -43,6 +43,13 @@ func init() { | |||
} | |||
conf.Ranges = ranges | |||
conf.Name = BalanceRegionName | |||
conf.Batch = 1 | |||
if len(args) >= 3 { |
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.
args could be greater than 3 because there could be multi ranges
oc.PromoteWaitingOperator() | ||
} else { | ||
for i := 0; i < added; i++ { | ||
oc.PromoteWaitingOperator() |
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.
should we check ops[id] is merge?
@@ -313,7 +312,17 @@ func (oc *OperatorController) AddWaitingOperator(ops ...*operator.Operator) int | |||
|
|||
oc.Unlock() | |||
operatorWaitCounter.WithLabelValues(ops[0].Desc(), "promote-add").Inc() | |||
oc.PromoteWaitingOperator() | |||
|
|||
if added > 0 { |
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.
return as soon as possible so the maintainer needn't keep too many conditions in mind. For example, here we could return when we find added=0 so we needn't keep in mind the condition add=0
anymore.
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.
Rest LGTM
…rs (#4652) ref #4008, ref #4610 speed up balance leader by batch Signed-off-by: Cabinfever_B <[email protected]> Co-authored-by: Ti Chi Robot <[email protected]>
close it? I think the current bottleneck is the store limit. you can reopen it if needed. |
Signed-off-by: bufferflies [email protected]
What problem does this PR solve?
fix #3744, scheduler can generate some operators in scheduler time
What is changed and how it works?
Check List
Tests
Unit test
Manual test (add detailed scripts or steps below)
Code changes
Side effects
Related changes
Release note