operator: rewrite move region related functions #1667

Luffbee · 2019-08-06T11:44:25Z

What problem does this PR solve?

Make logic of matchPeerSteps much more clear.
Fix bug: CreateMoveRegionOperator and matchPeerSteps don't check RejectLeader label.
Unify order of parameters and return values.

What is changed and how it works?

Abstract moveRegionSteps and reuse it in matchPeerSteps.
Abstract transferLeaderToAnySteps to select new leader.

Check List

Tests

Unit test

disksing · 2019-08-07T04:39:14Z

@Connor1996 PTAL

Connor1996

Please add and remove peer one by one. And also make sure the leader is the last to be removed.

Luffbee · 2019-08-07T08:11:14Z

/rebuild

Luffbee · 2019-08-07T08:31:01Z

@Connor1996 PTAL

server/schedule/operator/operator.go

Connor1996 · 2019-08-07T08:55:04Z

server/schedule/operator/operator.go

+
+	var steps = make([]OpStep, 0, len(addPeerSteps)*2+len(rmPeerSteps)+len(tlSteps))
+	i, j := 0, 0
+	for ; i < len(addPeerSteps) && j < len(rmPeerSteps); i, j = i+1, j+1 {


add some comments to illustrate why we want to generate steps like this?

Actually, I'm not very clear about why should we add and remove peers one by one. Could you give some reasons? I will add them to the comments.

I think it is not clear like before.

server/schedule/operator/operator.go

rleungx · 2019-08-07T09:56:37Z

server/schedule/operator/operator.go

+}
+
+// transferLeaderToAnySteps returns the first suitable store to become region leader,
+func transferLeaderToAnySteps(leaderID uint64, storeIDs []uint64, cluster Cluster) (OpKind, []OpStep) {


How about just returning OpStep?

If we regard this function as a black box, we should not assume it will only return a TransferLeader step if succeeded.

What's more, I'd like to make *Steps functions have similar returns. Now there are 3 such functions that have different returns:

transferLeaderToAnySteps(this function): no error.

CreateAddPeerSteps and CreateAddLightPeerSteps: no OpKind and error.

I will add an error for this function, because it may fail.
The other two functions are exported, so I didn't touch them.

server/schedule/operator/operator.go

rleungx · 2019-08-07T10:07:19Z

server/schedule/operator/operator.go

+
+	var steps = make([]OpStep, 0, len(addPeerSteps)*2+len(rmPeerSteps)+len(tlSteps))
+	i, j := 0, 0
+	for ; i < len(addPeerSteps) && j < len(rmPeerSteps); i, j = i+1, j+1 {


I think it is not clear like before.

Luffbee · 2019-08-08T09:11:07Z

@Connor1996 @rleungx PTAL

server/schedule/operator/operator.go

Co-Authored-By: Ryan Leung <rleungx@gmail.com>

Luffbee · 2019-08-09T06:25:15Z

/rebuild

codecov-io · 2019-08-09T08:20:19Z

Codecov Report

Merging #1667 into master will decrease coverage by 0.01%.
The diff coverage is 82.82%.

@@            Coverage Diff             @@
##           master    #1667      +/-   ##
==========================================
- Coverage   76.69%   76.68%   -0.02%     
==========================================
  Files         157      157              
  Lines       15489    15485       -4     
==========================================
- Hits        11880    11875       -5     
  Misses       2593     2593              
- Partials     1016     1017       +1

Impacted Files	Coverage Δ
server/checker/merge_checker.go	`77.04% <0%> (-1.29%)`	⬇️
server/schedule/operator/operator.go	`87.25% <83.67%> (-0.13%)`	⬇️
server/kv/etcd_kv.go	`70.12% <0%> (-9.1%)`	⬇️
server/schedulers/shuffle_hot_region.go	`58.97% <0%> (-6.42%)`	⬇️
server/schedulers/random_merge.go	`61.53% <0%> (-5.13%)`	⬇️
server/region_syncer/client.go	`81.01% <0%> (-2.54%)`	⬇️
server/tso/tso.go	`77.35% <0%> (-1.89%)`	⬇️
server/grpc_service.go	`58.09% <0%> (-0.44%)`	⬇️
client/client.go	`68.38% <0%> (+0.2%)`	⬆️
server/cluster.go	`82.76% <0%> (+0.25%)`	⬆️
... and 4 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 561d004...645ccfb. Read the comment docs.

server/schedule/operator/operator.go

…nto rewrite-move-region

Luffbee · 2019-08-13T02:59:03Z

/rebuild

nolouch

rest LGTM.

server/schedule/operator/operator.go

Luffbee · 2019-08-16T10:15:45Z

@Connor1996 @nolouch @rleungx PTAL

nolouch

LGTM

…nto rewrite-move-region

rleungx · 2019-08-20T02:34:14Z

server/schedule/operator/operator.go

+//  c == [opA1, opA2, opB1, opA3, opB2, opA4, opA5, opA6, opB3, opB4, opB5, opB6]
+//
+// sizeHint is a hint for the length of returned slice.
+func interleaveStepGroups(a, b [][]OpStep, sizeHint int) []OpStep {


Why we need this hint?

This hint is just like the second and third arguments in make(), for performance, not necessary.

Luffbee · 2019-08-20T02:54:01Z

/rebuild

Luffbee · 2019-08-20T03:01:09Z

/rebuild

Luffbee · 2019-08-20T03:18:18Z

/rebuild

rleungx · 2019-08-20T03:30:17Z

@Connor1996 PTAL.

Connor1996

LGTM

sre-bot · 2019-08-22T08:46:15Z

/run-all-tests

* *: unify get store function everywhere (#1671) Signed-off-by: Ryan Leung <rleungx@gmail.com> * server: use leader lease to determine tso service validity (#1676) Signed-off-by: disksing <i@disksing.com> * test: fix tests (#1696) * test: fix region syncer test Signed-off-by: disksing <i@disksing.com> * add config-check flag for pd-server (#1695) Signed-off-by: cwen0 <cwenyin0@gmail.com> * operator: rewrite move region related functions (#1667) * *: support setting endKey for ScanRange (#1700) Signed-off-by: disksing <i@disksing.com> * *: reduce some unnecessary parameters (#1698) Signed-off-by: Ryan Leung <rleungx@gmail.com> * schedule: Do not send an operator of a region wth a stale epoch (#1659) * schedule: Do not send an operator of a region wth a stale epoch Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: check the version changed by the operator self Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix unit test Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix to avoid dispatching a stale opstep Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refactor "ConsumeConfVer() int" to "ExpectConfVerChange() bool" Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: fix typo in comment Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * fix typo Co-Authored-By: Ryan Leung <rleungx@gmail.com> * dispatch: fix unittest Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refine format Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * server: fix the dead lock in scatter region (#1706) Signed-off-by: Ryan Leung <rleungx@gmail.com>

* *: unify get store function everywhere (#1671) Signed-off-by: Ryan Leung <rleungx@gmail.com> * remove unnecessary parentheses * server: use leader lease to determine tso service validity (#1676) Signed-off-by: disksing <i@disksing.com> * change internal stat values to float64 * add pending operator influence * add metrics of pending influence * fix metrics * fix panic * adjust pending influence of balanceHotWrite * change weight of pending influence * test: fix tests (#1696) * test: fix region syncer test Signed-off-by: disksing <i@disksing.com> * decrease region rolling window; store pending influence in scheduler * add config-check flag for pd-server (#1695) Signed-off-by: cwen0 <cwenyin0@gmail.com> * decrease possiblility transfer hot write leader * change pending influence weight * add unstarted op metrics * add logs for debug * add log for debug * add logs for debug * add logs for debug * add logs for debug * add logs for debug * add logs for debug * add logs for debug * Revert "add logs for debug" This reverts commit e74c7a9. * add metrics for hotspot operators * operator: rewrite move region related functions (#1667) * add metrics for pending operators * *: support setting endKey for ScanRange (#1700) Signed-off-by: disksing <i@disksing.com> * fix bug * fix bug * fix bug * fix metrics thread-safe bug * fix logic bug * *: reduce some unnecessary parameters (#1698) Signed-off-by: Ryan Leung <rleungx@gmail.com> * schedule: Do not send an operator of a region wth a stale epoch (#1659) * schedule: Do not send an operator of a region wth a stale epoch Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: check the version changed by the operator self Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix unit test Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix to avoid dispatching a stale opstep Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refactor "ConsumeConfVer() int" to "ExpectConfVerChange() bool" Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: fix typo in comment Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * fix typo Co-Authored-By: Ryan Leung <rleungx@gmail.com> * dispatch: fix unittest Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refine format Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * server: fix the dead lock in scatter region (#1706) Signed-off-by: Ryan Leung <rleungx@gmail.com> * add drop time for operator * use IsDropped to recognize canceled ops * try to fix trans leader burst * try to fix trans leader burst * add zombie influence * change select src dst strategy; improve op_controller * change select src strategy * fix bug * tools: fix set namespace in pd-ctl (#1701) Signed-off-by: Ryan Leung <rleungx@gmail.com> * tools: fix parse url without http prefix (#1703) Signed-off-by: Ryan Leung <rleungx@gmail.com> * tests: support deadlock detection in make test (#1704) Signed-off-by: Ryan Leung <rleungx@gmail.com> * Makefile: fix failpoint enable (#1722) Signed-off-by: nolouch <nolouch@gmail.com> * checker: fix the issue that a region does not merge to the sibling with smaller size (#1723) Signed-off-by: disksing <i@disksing.com> * tools: balance region simulator (#1708) * scheduler: do not remove the operator when the step does not finish (#1715) Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * operator: fix the AddLearner config version judgment (#1732) Signed-off-by: nolouch <nolouch@gmail.com> * tools: fix TLS in pd control (#1729) Signed-off-by: Ryan Leung <rleungx@gmail.com> * syncer: support TLS for region syncer (#1728) Signed-off-by: Ryan Leung <rleungx@gmail.com> * schedule: fix a thread-safe bug and improve code (#1719)

* *: unify get store function everywhere (#1671) Signed-off-by: Ryan Leung <rleungx@gmail.com> * server: use leader lease to determine tso service validity (#1676) Signed-off-by: disksing <i@disksing.com> * test: fix tests (#1696) * test: fix region syncer test Signed-off-by: disksing <i@disksing.com> * add config-check flag for pd-server (#1695) Signed-off-by: cwen0 <cwenyin0@gmail.com> * operator: rewrite move region related functions (#1667) * *: support setting endKey for ScanRange (#1700) Signed-off-by: disksing <i@disksing.com> * *: reduce some unnecessary parameters (#1698) Signed-off-by: Ryan Leung <rleungx@gmail.com> * schedule: Do not send an operator of a region wth a stale epoch (#1659) * schedule: Do not send an operator of a region wth a stale epoch Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: check the version changed by the operator self Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix unit test Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix to avoid dispatching a stale opstep Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refactor "ConsumeConfVer() int" to "ExpectConfVerChange() bool" Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: fix typo in comment Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * fix typo Co-Authored-By: Ryan Leung <rleungx@gmail.com> * dispatch: fix unittest Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refine format Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * server: fix the dead lock in scatter region (#1706) Signed-off-by: Ryan Leung <rleungx@gmail.com> * tools: fix set namespace in pd-ctl (#1701) Signed-off-by: Ryan Leung <rleungx@gmail.com> * tools: fix parse url without http prefix (#1703) Signed-off-by: Ryan Leung <rleungx@gmail.com> * tests: support deadlock detection in make test (#1704) Signed-off-by: Ryan Leung <rleungx@gmail.com> * Makefile: fix failpoint enable (#1722) Signed-off-by: nolouch <nolouch@gmail.com> * checker: fix the issue that a region does not merge to the sibling with smaller size (#1723) Signed-off-by: disksing <i@disksing.com> * tools: balance region simulator (#1708) * scheduler: do not remove the operator when the step does not finish (#1715) Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * operator: fix the AddLearner config version judgment (#1732) Signed-off-by: nolouch <nolouch@gmail.com> * tools: fix TLS in pd control (#1729) Signed-off-by: Ryan Leung <rleungx@gmail.com> * syncer: support TLS for region syncer (#1728) Signed-off-by: Ryan Leung <rleungx@gmail.com> * schedule: fix a thread-safe bug and improve code (#1719) * statistics: fix region flow calculation (#1688) Signed-off-by: jiyingtk <jiyingtk@mail.ustc.edu.cn> * makefile: improve deadlock-enable/disable (#1736) * api: fix missing keys statistic in region information (#1741) Signed-off-by: nolouch <nolouch@gmail.com> * *: update go version to 1.13 (#1742) Signed-off-by: disksing <i@disksing.com> * coordinator: add the operator cost time in log field (#1748) Signed-off-by: nolouch <nolouch@gmail.com>

Luffbee added 4 commits August 6, 2019 19:10

rewrite CreateMoveRegionOperator and matchPeerSteps

34d0d58

add label check for MoveRegion and matchPeer

3b9e828

fix misspelling

7e029a7

fix kind handling

58ebbab

Luffbee added 2 commits August 7, 2019 13:52

update moveRegionSteps

b37faff

fix text

337dc30

Connor1996 reviewed Aug 7, 2019

View reviewed changes

Luffbee added 2 commits August 7, 2019 15:15

add test for reject leader label

70cd43e

reorder steps of orderedMoveRegionSteps according to comment

b16fd81

Connor1996 reviewed Aug 7, 2019

View reviewed changes

rleungx reviewed Aug 7, 2019

View reviewed changes

Luffbee added 4 commits August 7, 2019 20:30

address comments

0920e49

improve comments

b739656

change parseStoreIDs back to map

b073942

Merge branch 'master' into rewrite-move-region

b7753bb

Luffbee changed the title ~~Rewrite some functions in 'schedule/operator'.~~ operator: rewrite move region related functions Aug 8, 2019

rleungx reviewed Aug 9, 2019

View reviewed changes

server/schedule/operator/operator.go Outdated Show resolved Hide resolved

server/schedule/operator/operator.go Outdated Show resolved Hide resolved

server/schedule/operator/operator.go Outdated Show resolved Hide resolved

Luffbee and others added 3 commits August 9, 2019 10:52

Update server/schedule/operator/operator.go

a257544

Co-Authored-By: Ryan Leung <rleungx@gmail.com>

Update server/schedule/operator/operator.go

5a0c970

Co-Authored-By: Ryan Leung <rleungx@gmail.com>

address log

d0df4e5

Merge branch 'master' into rewrite-move-region

402b582

rleungx reviewed Aug 9, 2019

View reviewed changes

server/schedule/operator/operator.go Show resolved Hide resolved

Luffbee added 2 commits August 9, 2019 17:50

add test for findNoLabelProperty

3756ba2

Merge branch 'rewrite-move-region' of https://github.com/Luffbee/pd i…

8ec469f

…nto rewrite-move-region

Merge branch 'master' into rewrite-move-region

eade3d2

nolouch reviewed Aug 16, 2019

View reviewed changes

server/schedule/operator/operator.go Outdated Show resolved Hide resolved

server/schedule/operator/operator.go Outdated Show resolved Hide resolved

server/schedule/operator/operator.go Outdated Show resolved Hide resolved

Luffbee added 3 commits August 16, 2019 17:54

format comments; rename

3e0377b

rename

1287ab8

go fmt

c0d2c3c

Merge branch 'master' into rewrite-move-region

c0ba889

nolouch approved these changes Aug 16, 2019

View reviewed changes

nolouch added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 16, 2019

Luffbee added 2 commits August 19, 2019 18:01

Merge branch 'master' into rewrite-move-region

d6316bf

Merge branch 'rewrite-move-region' of https://github.com/Luffbee/pd i…

645ccfb

…nto rewrite-move-region

Luffbee requested review from rleungx and Connor1996 August 19, 2019 11:36

Merge branch 'master' into rewrite-move-region

c6e20da

rleungx reviewed Aug 20, 2019

View reviewed changes

fix comment

700a70a

Connor1996 approved these changes Aug 20, 2019

View reviewed changes

Luffbee added 2 commits August 21, 2019 14:07

Merge branch 'master' into rewrite-move-region

cb9a3d1

Merge branch 'master' into rewrite-move-region

adab5e8

Luffbee added the status/can-merge Indicates a PR has been approved by a committer. label Aug 22, 2019

sre-bot merged commit 84f6a82 into tikv:master Aug 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator: rewrite move region related functions #1667

operator: rewrite move region related functions #1667

Luffbee commented Aug 6, 2019 •

edited

disksing commented Aug 7, 2019

Connor1996 left a comment

Luffbee commented Aug 7, 2019

Luffbee commented Aug 7, 2019

Connor1996 Aug 7, 2019

Luffbee Aug 7, 2019

rleungx Aug 7, 2019

rleungx Aug 7, 2019

Luffbee Aug 7, 2019

rleungx Aug 7, 2019

Luffbee commented Aug 8, 2019

Luffbee commented Aug 9, 2019

codecov-io commented Aug 9, 2019 •

edited

Luffbee commented Aug 13, 2019

nolouch left a comment

Luffbee commented Aug 16, 2019

nolouch left a comment

rleungx Aug 20, 2019

Luffbee Aug 20, 2019

Luffbee commented Aug 20, 2019

Luffbee commented Aug 20, 2019

Luffbee commented Aug 20, 2019

rleungx commented Aug 20, 2019

Connor1996 left a comment

sre-bot commented Aug 22, 2019

operator: rewrite move region related functions #1667

operator: rewrite move region related functions #1667

Conversation

Luffbee commented Aug 6, 2019 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

disksing commented Aug 7, 2019

Connor1996 left a comment

Choose a reason for hiding this comment

Luffbee commented Aug 7, 2019

Luffbee commented Aug 7, 2019

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

Luffbee commented Aug 8, 2019

Luffbee commented Aug 9, 2019

codecov-io commented Aug 9, 2019 • edited

Codecov Report

Luffbee commented Aug 13, 2019

nolouch left a comment

Choose a reason for hiding this comment

Luffbee commented Aug 16, 2019

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

Luffbee commented Aug 20, 2019

Luffbee commented Aug 20, 2019

Luffbee commented Aug 20, 2019

rleungx commented Aug 20, 2019

Connor1996 left a comment

Choose a reason for hiding this comment

sre-bot commented Aug 22, 2019

Luffbee commented Aug 6, 2019 •

edited

codecov-io commented Aug 9, 2019 •

edited