scheduler: allow `balance-leader-scheduler` generate multiple operators #4652

CabinfeverB · 2022-02-17T09:43:16Z

Signed-off-by: Cabinfever_B cabinfeveroier@gmail.com

What problem does this PR solve?

Issue Number: ref #4610
speed up balance leader

What is changed and how it works?

Add a Batch param like #4008 that the scheduler can generate multi operators.

speed up balance leader by batch

Check List

Tests

Unit test

Release note

Speed up balance leader

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2022-02-17T09:43:18Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

nolouch
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

codecov · 2022-02-17T13:39:01Z

Codecov Report

Merging #4652 (785ade2) into master (6d4d8a5) will increase coverage by 0.02%.
The diff coverage is 97.84%.

@@            Coverage Diff             @@
##           master    #4652      +/-   ##
==========================================
+ Coverage   75.03%   75.06%   +0.02%     
==========================================
  Files         284      284              
  Lines       27942    27985      +43     
==========================================
+ Hits        20966    21006      +40     
- Misses       5110     5111       +1     
- Partials     1866     1868       +2

Flag	Coverage Δ
unittests	`75.06% <97.84%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
server/schedulers/balance_leader.go	`92.50% <97.67%> (+0.83%)`	⬆️
server/cluster/coordinator.go	`74.17% <100.00%> (ø)`
server/schedule/operator_controller.go	`84.44% <100.00%> (-0.04%)`	⬇️
server/tso/local_allocator.go	`64.86% <0.00%> (-6.76%)`	⬇️
server/tso/tso.go	`61.14% <0.00%> (-5.72%)`	⬇️
pkg/etcdutil/etcdutil.go	`84.70% <0.00%> (-3.53%)`	⬇️
server/tso/allocator_manager.go	`63.99% <0.00%> (-1.79%)`	⬇️
server/election/lease.go	`86.15% <0.00%> (-1.54%)`	⬇️
server/schedule/operator/operator.go	`93.57% <0.00%> (-1.43%)`	⬇️
server/schedule/operator/step.go	`74.07% <0.00%> (-0.68%)`	⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d4d8a5...785ade2. Read the comment docs.

rleungx · 2022-02-18T10:15:43Z

server/schedule/operator_controller.go

@@ -821,6 +821,14 @@ func (oc *OperatorController) GetFastOpInfluence(cluster Cluster, influence oper
 	}
 }

+// AddOpInfluence add operator influence for cluster
+func AddOpInfluence(op *operator.Operator, influence operator.OpInfluence, cluster Cluster) {


What is it used for?

change store leader score to sort again

Could we use this function on L841-845 in the function NewTotalOpInfluence?

rleungx · 2022-02-18T10:17:13Z

server/schedulers/balance_leader.go

@@ -51,6 +51,13 @@ func init() {
 			}
 			conf.Ranges = ranges
 			conf.Name = BalanceLeaderName
+			conf.Batch = 1
+			if len(args) > len(ranges)*2 {


I think there is no need to change it.

rleungx · 2022-02-18T10:20:49Z

server/schedulers/balance_leader.go

@@ -161,48 +170,70 @@ func (l *balanceLeaderScheduler) Schedule(cluster schedule.Cluster) []*operator.
 			targets[j].LeaderScore(leaderSchedulePolicy, jOp)
 	})

+	usedStores := make(map[uint64]struct{})


I think the stores can be used more than once.

has changed

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

server/schedulers/balance_leader.go

CabinfeverB · 2022-03-08T08:38:15Z

/cc @HunDunDM

HunDunDM · 2022-03-08T12:35:22Z

server/schedulers/balance_leader.go

+	return result
+}
+
+func sortStores(stores []*core.StoreInfo, pos int, less func(i, j int) bool) {


How about resortStores?

HunDunDM · 2022-03-08T12:36:48Z

server/schedulers/balance_leader.go

+	sourcePoint := 0
+	targetPoint := 0


I think at least some comments are needed.

rafactor it

rleungx · 2022-03-09T03:05:26Z

server/schedulers/balance_leader.go

@@ -51,6 +51,7 @@ func init() {
 			}
 			conf.Ranges = ranges
 			conf.Name = BalanceLeaderName
+			conf.Batch = 5


how about using a constant?

rleungx · 2022-03-09T03:10:53Z

server/schedulers/balance_leader.go

+	for sourcePoint < len(sources) || targetPoint < len(targets) {
+		if sourcePoint < len(sources) {
+			used := false
+			plan.source, plan.target = sources[sourcePoint], nil
 			retryLimit := l.retryQuota.GetLimit(plan.source)
 			log.Debug("store leader score", zap.String("scheduler", l.GetName()), zap.Uint64("source-store", plan.SourceStoreID()))
 			l.counter.WithLabelValues("high-score", plan.SourceMetricLabel()).Inc()
 			for j := 0; j < retryLimit; j++ {
 				schedulerCounter.WithLabelValues(l.GetName(), "total").Inc()
 				if ops := l.transferLeaderOut(plan); len(ops) > 0 {


How about changing it to op?

rleungx · 2022-03-09T03:11:23Z

server/schedulers/balance_leader.go

+	for sourcePoint < len(sources) || targetPoint < len(targets) {
+		if sourcePoint < len(sources) {
+			used := false
+			plan.source, plan.target = sources[sourcePoint], nil


Maybe index?

yep, updated

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

AndreMouche · 2022-03-09T09:52:42Z

server/schedule/operator_controller.go

@@ -821,6 +821,14 @@ func (oc *OperatorController) GetFastOpInfluence(cluster Cluster, influence oper
 	}
 }

+// AddOpInfluence add operator influence for cluster
+func AddOpInfluence(op *operator.Operator, influence operator.OpInfluence, cluster Cluster) {


Could we use this function on L841-845 in the function NewTotalOpInfluence?

AndreMouche · 2022-03-09T10:05:14Z

server/schedulers/balance_leader.go

+	sort.Slice(bc.targets, func(i, j int) bool {
+		iOp := bc.plan.GetOpInfluence(bc.targets[i].GetID())
+		jOp := bc.plan.GetOpInfluence(bc.targets[j].GetID())
+		return bc.targets[i].LeaderScore(bc.plan.kind.Policy, iOp) <


How about defining two functions? With the name of the function, the maintainer can easily find the slice is ordered by desc or asc.

AndreMouche · 2022-03-09T10:07:09Z

server/schedulers/balance_leader.go

+
+// hasOptionalStore returns whether there is remaining and optional stores
+func (bc *batchController) hasOptionalStore() bool {
+	return bc.sourceIndex < len(bc.sources) || bc.targetIndex < len(bc.targets)


Suggested change

return bc.sourceIndex < len(bc.sources) || bc.targetIndex < len(bc.targets)

return bc.hasOptionalSourceStore() || bc.hasOptionalTargetStore()

AndreMouche · 2022-03-09T10:14:18Z

server/schedulers/balance_leader.go

+	return bc.sourceIndex < len(bc.sources) || bc.targetIndex < len(bc.targets)
+}
+
+// hasOptionalSourceStore returns whether there is remaining and optional source stores


How about

`hasSourceStore` returns true when there are leftover source stores.

rleungx · 2022-03-09T11:24:06Z

Some functions are only used once and it's so easy. For these functions, adding some comments is enough.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

…leader

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch

lgtm

rleungx · 2022-03-14T02:46:41Z

server/schedulers/balance_leader.go

+	store := cs.getStore()
+	retryLimit := l.retryQuota.GetLimit(store)
+	var creator func(*balancePlan) *operator.Operator
+	log.Debug("store leader score", zap.String("scheduler", l.GetName()), zap.Uint64(dir, store.GetID()))


no score field?

This statement didn't have it before, but it's better to add it.

rleungx · 2022-03-14T02:51:11Z

server/schedulers/balance_leader.go

+		l.counter.WithLabelValues("low-score", plan.TargetMetricLabel()).Inc()
+		creator = l.transferLeaderIn
+	}
+	var op *operator.Operator = nil


Suggested change

var op *operator.Operator = nil

var op *operator.Operator

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-03-14T04:09:03Z

server/schedulers/balance_leader.go

+	}
+}
+
+func leaderScore(store *core.StoreInfo, plan *balancePlan) float64 {


I prefer to remove this function and move GetOpInfluence directly into xxxOption.

rleungx

The rest LGTM

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch · 2022-03-14T07:50:44Z

/merge

ti-chi-bot · 2022-03-14T07:50:45Z

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 ti-community-infra/tichi repository.

ti-chi-bot · 2022-03-14T07:50:47Z

This pull request has been accepted and is ready to merge.

Commit hash: 9a32d17

ref #4610, ref #4652 Add `balance-leader-leader` config API. Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

ref #4610, ref #4652, ref #4655 pdctl supports update balance-leader-scheduler config Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

ref tikv#4610, ref tikv#4652, ref tikv#4655 pdctl supports update balance-leader-scheduler config Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

speed up balance leader

ab6f582

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 17, 2022

ti-chi-bot requested review from disksing and rleungx February 17, 2022 09:43

CabinfeverB added 2 commits February 17, 2022 18:18

fix test

f5be6dd

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix scheduleController

263cc1c

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB mentioned this pull request Feb 18, 2022

Speed up balance leader #4610

Closed

CabinfeverB marked this pull request as ready for review February 18, 2022 05:41

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2022

CabinfeverB changed the title ~~speed up balance leader~~ scheduler: allow balance-leader-scheduler generate multiple operators Feb 18, 2022

rleungx reviewed Feb 18, 2022

View reviewed changes

CabinfeverB added 2 commits March 6, 2022 23:48

Merge branch 'master' into speed_up_balance_leader

0976e10

change store uesd limit

7798e88

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch reviewed Mar 8, 2022

View reviewed changes

server/schedulers/balance_leader.go Outdated Show resolved Hide resolved

ti-chi-bot requested a review from HunDunDM March 8, 2022 08:38

HunDunDM reviewed Mar 8, 2022

View reviewed changes

rleungx reviewed Mar 9, 2022

View reviewed changes

CabinfeverB added 3 commits March 9, 2022 15:31

refactor

0528f86

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

refactor controller

3f139b8

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

refactor controller

065f012

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

AndreMouche reviewed Mar 9, 2022

View reviewed changes

CabinfeverB added 3 commits March 10, 2022 17:15

refactor controller

15925f9

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Merge remote-tracking branch 'upstream/master' into speed_up_balance_…

f55e76b

…leader

simply

6f9232b

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB mentioned this pull request Mar 11, 2022

api, scheduler: add balance-leader config handler #4655

Merged

CabinfeverB mentioned this pull request Mar 11, 2022

pdctl: support config balance-leader #4656

Merged

CabinfeverB added 3 commits March 13, 2022 15:09

Merge branch 'master' into speed_up_balance_leader

44b23ae

refactor

594ecec

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

refactor

2cd7b9c

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch approved these changes Mar 14, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 14, 2022

rleungx reviewed Mar 14, 2022

View reviewed changes

CabinfeverB added 3 commits March 14, 2022 11:19

add debug score

30fd878

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

new func

efbb655

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

new func

b165436

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx reviewed Mar 14, 2022

View reviewed changes

remove debug log

9a32d17

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx approved these changes Mar 14, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 14, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 14, 2022

Merge branch 'master' into speed_up_balance_leader

785ade2

ti-chi-bot merged commit c78c652 into tikv:master Mar 14, 2022

CabinfeverB mentioned this pull request Jul 14, 2022

should use UnfinishedInfluence in GetOpInfluence #5311

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: allow `balance-leader-scheduler` generate multiple operators #4652

scheduler: allow `balance-leader-scheduler` generate multiple operators #4652

CabinfeverB commented Feb 17, 2022 •

edited

Loading

ti-chi-bot commented Feb 17, 2022 •

edited

Loading

codecov bot commented Feb 17, 2022 •

edited

Loading

rleungx Feb 18, 2022

CabinfeverB Mar 7, 2022

AndreMouche Mar 9, 2022

rleungx Feb 18, 2022

rleungx Feb 18, 2022

CabinfeverB Mar 7, 2022

CabinfeverB commented Mar 8, 2022

HunDunDM Mar 8, 2022

CabinfeverB Mar 9, 2022

HunDunDM Mar 8, 2022

CabinfeverB Mar 9, 2022

rleungx Mar 9, 2022

CabinfeverB Mar 9, 2022

rleungx Mar 9, 2022

CabinfeverB Mar 9, 2022

rleungx Mar 9, 2022

CabinfeverB Mar 9, 2022

AndreMouche Mar 9, 2022

AndreMouche Mar 9, 2022

AndreMouche Mar 9, 2022

AndreMouche Mar 9, 2022

rleungx commented Mar 9, 2022

nolouch left a comment

rleungx Mar 14, 2022

CabinfeverB Mar 14, 2022

rleungx Mar 14, 2022

rleungx Mar 14, 2022

rleungx left a comment

nolouch commented Mar 14, 2022

ti-chi-bot commented Mar 14, 2022

ti-chi-bot commented Mar 14, 2022

	return bc.sourceIndex < len(bc.sources) \|\| bc.targetIndex < len(bc.targets)
	return bc.hasOptionalSourceStore() \|\| bc.hasOptionalTargetStore()

scheduler: allow balance-leader-scheduler generate multiple operators #4652

scheduler: allow balance-leader-scheduler generate multiple operators #4652

Conversation

CabinfeverB commented Feb 17, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Feb 17, 2022 • edited Loading

codecov bot commented Feb 17, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CabinfeverB commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx commented Mar 9, 2022

nolouch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

nolouch commented Mar 14, 2022

ti-chi-bot commented Mar 14, 2022

ti-chi-bot commented Mar 14, 2022

scheduler: allow `balance-leader-scheduler` generate multiple operators #4652

scheduler: allow `balance-leader-scheduler` generate multiple operators #4652

CabinfeverB commented Feb 17, 2022 •

edited

Loading

ti-chi-bot commented Feb 17, 2022 •

edited

Loading

codecov bot commented Feb 17, 2022 •

edited

Loading