dm-master: leader election #367

csuzhangxc · 2019-11-20T10:14:53Z

What problem does this PR solve?

add leader election supported for DM-master based on embed etcd

What is changed and how it works?

do leader election based on embed etcd

some code learn from https://github.com/pingcap/tidb/blob/v3.0.5/owner/manager.go.

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

Need to be included in the release note

csuzhangxc · 2019-11-20T10:15:20Z

/run-all-tests tidb=release-3.0

codecov · 2019-11-20T10:22:49Z

Codecov Report

Merging #367 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #367   +/-   ##
===========================================
  Coverage   57.6674%   57.6674%           
===========================================
  Files           162        162           
  Lines         16368      16368           
===========================================
  Hits           9439       9439           
  Misses         6006       6006           
  Partials        923        923

…ter-leader-election

csuzhangxc · 2019-11-20T13:21:11Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-20T13:47:58Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-20T14:04:53Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-20T14:30:11Z

@WangXiangUSTC , @GregoryIan or @amyangfei PTAL

csuzhangxc · 2019-11-21T00:33:34Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-21T00:43:33Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-21T02:43:01Z

I update the methods for Election after added PTAL label. It's ready for review again now.

csuzhangxc · 2019-11-21T02:48:25Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-21T02:56:57Z

tests/others_integration.txt

-all_mode
-sequence_sharding
-sequence_safe_mode
-relay_interrupt
 start_task
-initial_unit
-http_apis


I have updated the CI to run these cases separately

WangXiangUSTC · 2019-11-21T03:48:11Z

pkg/election/election.go

+
+			for _, ev := range resp.Events {
+				if ev.Type == mvccpb.DELETE {
+					e.l.Info("fail to watch, the leader is deleted", zap.ByteString("key", ev.Kv.Key))


if the type is DELETE, dose it mean the electionTTL is overdue？

user may force to trigger a new campaign by deleting the key with etcdctl (or dmctl in the future); in the normal case, before electionTTL overdue, Session will keep alive for the lifetime of a client, so this will not get DELETE event.

see:

https://github.com/etcd-io/etcd/blob/v3.4.3/clientv3/concurrency/session.go#L26

https://github.com/etcd-io/etcd/blob/v3.4.3/clientv3/concurrency/session.go#L63

how about adding a comment in which case will get delete event

added in b99ae46.

WangXiangUSTC · 2019-11-21T05:04:45Z

dm/master/election.go

+	"github.com/pingcap/dm/pkg/log"
+)
+
+func (s *Server) electionNotify(ctx context.Context) {


why need this function, seems toBeLeader and retireLeader already print the log

toBeLeader and retireLeader only log the leadership of the current member.
this function is used to show the usage of Election (and also log the leader name even if it's not the current member), do you think should we need to remove it?

one more thing, pkg/election is a pkg, but this function is in the application. Maybe we need to add some metrics for the leadership later, and do that in this function is better.

so, how about removing the log in pkg/election?

if master needs to do something(for example: meet error in the election, master exit) according to the election's result I think putting here is better.
it is up to you

I chose to remove the log in pkg/election in b99ae46.

pkg/election/election.go

WangXiangUSTC · 2019-11-21T05:16:29Z

dm/master/server.go

 	s.closed.Set(false) // the server started now.

+	// start leader election
+	s.election = election.NewElection(ctx, s.etcdClient, electionTTL, electionKey, s.cfg.Name)


do we need to handle election's error from election.ErrorNotify()

I update Election to let it more like which one in TiDB's owner pkg in 80ae3d2 and 5c4c820.

Detect any potential error (for unrecoverable error) in NewElection, and do not start Server if an error occurred.

Retry for errors (for recoverable error) until the context is done after entering the campaign loop.

receive errors from election.ErrorNotify() in server/election.go, but do no meaningful things now.

csuzhangxc · 2019-11-21T08:13:36Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-21T08:29:58Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-21T08:36:39Z

@WangXiangUSTC PTAL again

WangXiangUSTC

rest LGTM

WangXiangUSTC · 2019-11-21T11:19:29Z

pkg/election/election.go

+			select {
+			case <-time.After(newSessionRetryInterval):
+			case <-ctx.Done():
+				break


do you means break the for loop, this break only break the select
https://stackoverflow.com/questions/11104085/in-go-does-a-break-statement-break-from-a-switch-select

good catch!, fixed in b99ae46.

csuzhangxc · 2019-11-21T11:45:31Z

/run-all-tests tidb=release-3.0

WangXiangUSTC · 2019-11-21T11:49:16Z

LGTM

pkg/election/election.go

dm/master/election.go

…er-election # Conflicts: # tests/others_integration.txt

csuzhangxc · 2019-11-25T11:54:28Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-25T12:00:10Z

/run-all-tests tidb=release-3.0

amyangfei

LGTM

csuzhangxc added 4 commits November 20, 2019 13:19

*: add etcd leader election

1464ce3

*: support get member's ID and leader's ID

e7e0ad9

election: add election test case; refine code

45e8d3b

election: fix key watch; add more tests

1d0b22d

Merge branch 'master' into master-leader-election

bc54f9d

csuzhangxc added 4 commits November 20, 2019 20:14

election: add more tests

ede2549

*: extract CreateClient for etcd

792b41c

master: elect leader when starting

3179fc3

Merge remote-tracking branch 'origin/master-leader-election' into mas…

1d47595

…ter-leader-election

election: add some log

7d45b14

csuzhangxc added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Nov 20, 2019

csuzhangxc requested review from amyangfei, WangXiangUSTC and IANTHEREAL November 20, 2019 14:30

election: remove useless code

12385f3

*: fix CI

d2059a6

election: refactor leader notify API

f2540ef

tests: update others_integration.txt to match the CI

dbb87d0

csuzhangxc commented Nov 21, 2019

View reviewed changes

csuzhangxc mentioned this pull request Nov 21, 2019

master: use etcd as operate queue #363

Closed

WangXiangUSTC reviewed Nov 21, 2019

View reviewed changes

*: address comment

80ae3d2

*: address comment

5c4c820

WangXiangUSTC reviewed Nov 21, 2019

View reviewed changes

election: address comments

b99ae46

WangXiangUSTC added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Nov 21, 2019

amyangfei reviewed Nov 25, 2019

View reviewed changes

pkg/election/election.go Outdated Show resolved Hide resolved

dm/master/election.go Outdated Show resolved Hide resolved

csuzhangxc added 2 commits November 25, 2019 19:25

*: address comments

708c2c1

Merge remote-tracking branch 'remotes/origin/master' into master-lead…

93f6b07

…er-election # Conflicts: # tests/others_integration.txt

amyangfei approved these changes Nov 26, 2019

View reviewed changes

amyangfei added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Nov 26, 2019

csuzhangxc merged commit 18f66f2 into pingcap:master Nov 26, 2019

csuzhangxc deleted the master-leader-election branch November 26, 2019 15:00

csuzhangxc mentioned this pull request Dec 6, 2019

Make DM become HA #406

Open

23 tasks

lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020

dm-master: leader election (pingcap#367)

eb766d3

csuzhangxc removed the needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated label Jun 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dm-master: leader election #367

dm-master: leader election #367

csuzhangxc commented Nov 20, 2019 •

edited

Loading

csuzhangxc commented Nov 20, 2019

codecov bot commented Nov 20, 2019 •

edited

Loading

csuzhangxc commented Nov 20, 2019

csuzhangxc commented Nov 20, 2019

csuzhangxc commented Nov 20, 2019

csuzhangxc commented Nov 20, 2019

csuzhangxc commented Nov 21, 2019

csuzhangxc commented Nov 21, 2019

csuzhangxc commented Nov 21, 2019

csuzhangxc commented Nov 21, 2019

csuzhangxc Nov 21, 2019

WangXiangUSTC Nov 21, 2019

csuzhangxc Nov 21, 2019 •

edited

Loading

WangXiangUSTC Nov 21, 2019

csuzhangxc Nov 21, 2019

WangXiangUSTC Nov 21, 2019

csuzhangxc Nov 21, 2019

csuzhangxc Nov 21, 2019 •

edited

Loading

WangXiangUSTC Nov 21, 2019

csuzhangxc Nov 21, 2019

WangXiangUSTC Nov 21, 2019

csuzhangxc Nov 21, 2019

csuzhangxc commented Nov 21, 2019

csuzhangxc commented Nov 21, 2019

csuzhangxc commented Nov 21, 2019

WangXiangUSTC left a comment

WangXiangUSTC Nov 21, 2019

csuzhangxc Nov 21, 2019

csuzhangxc commented Nov 21, 2019

WangXiangUSTC commented Nov 21, 2019

csuzhangxc commented Nov 25, 2019

csuzhangxc commented Nov 25, 2019

amyangfei left a comment

dm-master: leader election #367

dm-master: leader election #367

Conversation

csuzhangxc commented Nov 20, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

csuzhangxc commented Nov 20, 2019

codecov bot commented Nov 20, 2019 • edited Loading

Codecov Report

csuzhangxc commented Nov 20, 2019

csuzhangxc commented Nov 20, 2019

csuzhangxc commented Nov 20, 2019

csuzhangxc commented Nov 20, 2019

csuzhangxc commented Nov 21, 2019

csuzhangxc commented Nov 21, 2019

csuzhangxc commented Nov 21, 2019

csuzhangxc commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csuzhangxc Nov 21, 2019 • edited Loading

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

csuzhangxc Nov 21, 2019 • edited Loading

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

csuzhangxc commented Nov 21, 2019

csuzhangxc commented Nov 21, 2019

csuzhangxc commented Nov 21, 2019

WangXiangUSTC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csuzhangxc commented Nov 21, 2019

WangXiangUSTC commented Nov 21, 2019

csuzhangxc commented Nov 25, 2019

csuzhangxc commented Nov 25, 2019

amyangfei left a comment

Choose a reason for hiding this comment

csuzhangxc commented Nov 20, 2019 •

edited

Loading

codecov bot commented Nov 20, 2019 •

edited

Loading

csuzhangxc Nov 21, 2019 •

edited

Loading

csuzhangxc Nov 21, 2019 •

edited

Loading