schedule: fix a thread-safe bug and improve code #1719

Luffbee · 2019-08-30T15:28:21Z

What problem does this PR solve?

There is a thread-safe bug which may remove operators unexpectedly, see issue Operator may be removed unexpectedly. #1716.
There is a suspicious code in pollNeedDispathOperator: when the region of an operator is disappeared, it is ignored. The code to handle this problem is in RaftCluster.checkOperator, which is far away from the problem.

What is changed and how it works?

This PR use the second possible solution mentioned in the issue: check operator is the same before delete.
Moved the logic in RaftCluster.checkOperator to pollNeedDispathOperator, and removed some useless functions.

Check List

Tests

Unit test

codecov-io · 2019-08-30T15:38:08Z

Codecov Report

Merging #1719 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1719      +/-   ##
==========================================
+ Coverage   76.92%   76.95%   +0.02%     
==========================================
  Files         161      161              
  Lines       15734    15724      -10     
==========================================
- Hits        12103    12100       -3     
+ Misses       2613     2610       -3     
+ Partials     1018     1014       -4

Impacted Files	Coverage Δ
server/cluster.go	`83.91% <ø> (+1.06%)`	⬆️
server/schedule/operator_controller.go	`90.88% <100%> (+1.05%)`	⬆️
server/handler.go	`50.13% <100%> (+0.52%)`	⬆️
pkg/metricutil/metricutil.go	`90.62% <0%> (-9.38%)`	⬇️
server/schedulers/shuffle_hot_region.go	`58.97% <0%> (-6.42%)`	⬇️
server/region_syncer/client.go	`78.94% <0%> (-3.95%)`	⬇️
client/client.go	`69.76% <0%> (-0.65%)`	⬇️
server/schedule/operator/operator.go	`85.58% <0%> (+0.36%)`	⬆️
server/member/leader.go	`78.06% <0%> (+1.02%)`	⬆️

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 f2487bf...6a4ce79. Read the comment docs.

server/schedule/operator_controller_test.go

server/schedule/operator_controller.go

nolouch

LGTM

nolouch · 2019-09-04T06:01:29Z

PTAL @rleungx @shafreeck

server/handler.go

sre-bot · 2019-09-05T04:10:34Z

/run-all-tests

sre-bot · 2019-09-05T04:13:35Z

@Luffbee merge failed.

Luffbee · 2019-09-05T04:53:40Z

/rebuild

Luffbee · 2019-09-05T04:55:04Z

/run-all-tests

nolouch · 2019-09-05T06:25:55Z

/run-integration-common-test

server/schedule/operator_controller.go

nolouch · 2019-09-05T10:00:24Z

/run-integration-common-test

nolouch · 2019-09-06T06:34:55Z

/merge

rleungx · 2019-09-06T06:51:46Z

@Luffbee Please solve the conflicts.

rleungx · 2019-09-06T08:52:17Z

/merge

sre-bot · 2019-09-06T08:52:50Z

/run-all-tests

sre-bot · 2019-09-06T08:57:25Z

cherry pick to release-3.0 failed

* *: 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 12 commits August 29, 2019 20:09

dispatch finished operator when poll

fc868a1

add check for remove op

616c6ae

add return value for removeOperatorLocked

c9ba490

remove checkOperator of RaftCluster

e80f45d

remove RemoveTimeoutOperator

7aead13

enhance metrics tag

c3a6a74

enhance tests

b050731

add test for region disappear

501a58f

add test for poll finished op

829ffa2

fix a mistake in test

41d40c8

add test for concurrent remove operator

0efd082

Merge branch 'master' into fix-op-controller

d8ff405

Luffbee requested review from rleungx and nolouch September 2, 2019 03:12

nolouch added type/bug The issue is confirmed as a bug. needs-cherry-pick-release-3.0 The PR needs to cherry pick to release-3.0 branch. labels Sep 2, 2019

nolouch reviewed Sep 2, 2019

View reviewed changes

server/schedule/operator_controller_test.go Show resolved Hide resolved

rleungx reviewed Sep 2, 2019

View reviewed changes

server/schedule/operator_controller.go Show resolved Hide resolved

nolouch approved these changes Sep 2, 2019

View reviewed changes

rleungx reviewed Sep 4, 2019

View reviewed changes

server/handler.go Show resolved Hide resolved

Merge branch 'master' into fix-op-controller

a1b20f0

rleungx approved these changes Sep 5, 2019

View reviewed changes

rleungx added the status/can-merge Indicates a PR has been approved by a committer. label Sep 5, 2019

shafreeck reviewed Sep 5, 2019

View reviewed changes

server/schedule/operator_controller.go Show resolved Hide resolved

Merge branch 'master' into fix-op-controller

6a4ce79

sre-bot merged commit b66ba44 into tikv:master Sep 6, 2019

This was referenced Sep 6, 2019

schedule: fix a thread-safe bug and improve code (#1719) #1734

Merged

Operator may be removed unexpectedly. #1716

Closed

Luffbee deleted the fix-op-controller branch September 11, 2019 05:17

sre-bot pushed a commit that referenced this pull request Sep 11, 2019

schedule: fix a thread-safe bug and improve code (#1719) (#1734)

e07854f

sre-bot mentioned this pull request Sep 11, 2019

schedule: fix a thread-safe bug and improve code (#1719) (#1734) #1751

Merged

nolouch pushed a commit that referenced this pull request Sep 29, 2019

schedule: fix a thread-safe bug and improve code (#1719) (#1734) (#1751)

830ebf5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schedule: fix a thread-safe bug and improve code #1719

schedule: fix a thread-safe bug and improve code #1719

Luffbee commented Aug 30, 2019

codecov-io commented Aug 30, 2019 •

edited

Loading

nolouch left a comment

nolouch commented Sep 4, 2019

sre-bot commented Sep 5, 2019

sre-bot commented Sep 5, 2019

Luffbee commented Sep 5, 2019

Luffbee commented Sep 5, 2019

nolouch commented Sep 5, 2019

nolouch commented Sep 5, 2019

nolouch commented Sep 6, 2019

rleungx commented Sep 6, 2019

rleungx commented Sep 6, 2019

sre-bot commented Sep 6, 2019

sre-bot commented Sep 6, 2019

schedule: fix a thread-safe bug and improve code #1719

schedule: fix a thread-safe bug and improve code #1719

Conversation

Luffbee commented Aug 30, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

codecov-io commented Aug 30, 2019 • edited Loading

Codecov Report

nolouch left a comment

Choose a reason for hiding this comment

nolouch commented Sep 4, 2019

sre-bot commented Sep 5, 2019

sre-bot commented Sep 5, 2019

Luffbee commented Sep 5, 2019

Luffbee commented Sep 5, 2019

nolouch commented Sep 5, 2019

nolouch commented Sep 5, 2019

nolouch commented Sep 6, 2019

rleungx commented Sep 6, 2019

rleungx commented Sep 6, 2019

sre-bot commented Sep 6, 2019

sre-bot commented Sep 6, 2019

codecov-io commented Aug 30, 2019 •

edited

Loading