scheduler: do not remove the operator when the step does not finish

[shafreeck]

Signed-off-by: Shafreeck Sea shafreeck@gmail.com

What problem does this PR solve?

There is a bug introduced by #1652, in some case, like adding peers or adding learners, the step is left unfinished if the peer is in pending state, although the conf version has changed, in these cases, the operator will be removed because the controller thought someone has changed the conf version(in fact, it self did). We fix that by checking if the conf version has actually changed by current step, if it is, the operator is not regarded as stale.

What is changed and how it works?

Only check conf version changes when a new step to be executed.

Check List

Tests

• Unit test

[Luffbee]

A step may be send multiple times. If we only check ConfVer at the first time, the problem in the origin issue will appear again. (Consider if the ConfVer changed after the first send)

[codecov-io]

Codecov Report

Merging #1715 into master will decrease coverage by 0.01%.
The diff coverage is 68.75%.

@@            Coverage Diff             @@
##           master    #1715      +/-   ##
==========================================
- Coverage   76.91%   76.89%   -0.02%     
==========================================
  Files         160      160              
  Lines       15718    15730      +12     
==========================================
+ Hits        12089    12096       +7     
- Misses       2615     2619       +4     
- Partials     1014     1015       +1

Impacted Files	Coverage Δ
server/schedule/operator_controller.go	89.37% <100%> (ø)	⬆️
server/schedule/operator/operator.go	85.94% <67.74%> (-0.06%)	⬇️
server/tso/tso.go	77.06% <0%> (-6.43%)	⬇️
server/schedulers/random_merge.go	61.53% <0%> (-5.13%)	⬇️
pkg/etcdutil/etcdutil.go	88.4% <0%> (-2.9%)	⬇️
server/grpc_service.go	57.48% <0%> (-1.31%)	⬇️
server/server.go	82.21% <0%> (-0.58%)	⬇️
server/cluster.go	83.1% <0%> (+0.25%)	⬆️
server/member/leader.go	80.1% <0%> (+0.51%)	⬆️
server/handler.go	50.13% <0%> (+0.52%)	⬆️
... and 3 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 9af07b0...b5b2a3f. Read the comment docs.

[Luffbee]

What about merging ConfVerChanged and IsFinish? These two functions are too intimate, so separating them is not a good idea.

Then Check can return step and the total change, but it will need to scan steps from start. The problem here is: is it possible that a finished step becomes unfinished before the whole operator finished? (@nolouch). Actually, this problem also influences current implementation.

[nolouch]

@Luffbee Only if there are two operator works on one region at the same time. consider the network, It's possible but the probability is very small. But theepoch of the region will monotonically increase, so as your suggestion may better than now.

Unexpected approve.

[shafreeck]

@Luffbee I thought about your suggestion before and gave up. From the perspective of code, they really can be merged, but it is weird according to the semantics. Conf version is not the completion of an epoch. It is acceptable to return if the epoched changed like

func IsFinish(region) (epochChanged bool, finished bool)

It is weird like this

func IsFinish(region) (confVerChanged bool, finished bool)

If there is another requirement to check if a version changed, the latter signature maybe should look like this:

func IsFinish(region) (confVerChanged bool, versionChagned bool, finished bool)

I don't think this is a good trend.

[nolouch]

LGTM. let's merge this firstly. this changed not affect the old code style.

[nolouch]

PTAL @Luffbee @rleungx

[Luffbee]

LGTM

[sre-bot]

/run-all-tests