master: use etcd as operate queue #363

WangXiangUSTC · 2019-11-18T05:09:43Z

What problem does this PR solve?

use etcd as operate queue:

What is changed and how it works?

when dm-master receive operate request, save it to etcd, and watch the key for response
dm-master watch the path /dm-operate. when getting events, will handle the request if the dm-master is leader.

Check List

Tests

Unit test
Integration test
Manual test

Side effects

Breaking backward compatibility

Related changes

Need to be included in the release note

…quest in etcd

WangXiangUSTC · 2019-11-20T04:27:45Z

/run-all-tests

WangXiangUSTC · 2019-11-20T04:53:05Z

/run-all-tests tidb=release-3.0

codecov · 2019-11-20T05:00:15Z

Codecov Report

Merging #363 into master will decrease coverage by 0.4939%.
The diff coverage is 37.6257%.

@@               Coverage Diff               @@
##             master       #363       +/-   ##
===============================================
- Coverage   57.5617%   57.0677%   -0.494%     
===============================================
  Files           163        164        +1     
  Lines         16471      16936      +465     
===============================================
+ Hits           9481       9665      +184     
- Misses         6065       6290      +225     
- Partials        925        981       +56

WangXiangUSTC · 2019-11-20T09:37:19Z

/run-all-tests tidb=release-3.0

WangXiangUSTC · 2019-11-20T09:46:50Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-21T03:36:53Z

dm/master/etcd.go

+
+	defaultOperatePath = "/dm-operate"
+
+	defaultEtcdTimeout = time.Duration(10 * time.Second)


I also defined this in pkg/etcdutil/etcdutil.go. and and a CreateClient in it in #367 . we can merge then later.

ok, if your pr merged first I will use your function.

@WangXiangUSTC It's time to update this defaultEtcdTimeout with the one in pkg/etcdutil/etcdutil.go now?

dm/master/etcd.go

pkg/terror/error_list.go

dm/proto/operate.proto

csuzhangxc · 2019-11-21T04:05:57Z

dm/master/server_test.go

 )

 func TestMaster(t *testing.T) {
+	etcdMockCluster := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 1})


dm/master/server.go

WangXiangUSTC · 2019-12-02T15:42:40Z

/run-all-tests tidb=release-3.0

WangXiangUSTC · 2019-12-02T16:26:50Z

/run-all-tests tidb=release-3.0

WangXiangUSTC · 2019-12-02T16:37:07Z

/run-all-tests tidb=release-3.0

WangXiangUSTC · 2019-12-03T03:47:25Z

/run-all-tests tidb=release-3.0

WangXiangUSTC · 2019-12-03T04:01:03Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-12-03T07:02:47Z

dm/ctl/master/show_leader.go

+	"github.com/pingcap/dm/dm/pb"
+)
+
+// NewShowLeaderCmd creates a CheckTask command


please update the comment (not CheckTask)

csuzhangxc · 2019-12-03T07:25:03Z

tests/dmctl_basic/check_list/check_master_leader.sh

+function show_master_leader_success() {
+    run_dm_ctl $WORK_DIR "127.0.0.1:$MASTER_PORT" \
+        "show-master-leader" \
+        "\"result\": true" 1


How about checking any successful msg?

add in 20524e7

csuzhangxc · 2019-12-03T07:46:49Z

dm/proto/dmmaster.proto

+message ShowMasterLeaderRequest {
+}
+
+// If have leader, result is true, and put leader information in msg.


How about defining the leader information as another message?

add in 20524e7

csuzhangxc · 2019-12-03T07:49:43Z

dm/proto/operate.proto

+}
+
+enum OperateStage {
+    UnknownStage = 0;


Do we need another stage like New to differ with the default Unknown?

add in 20524e7

csuzhangxc · 2019-12-03T07:56:17Z

dm/master/server_test.go

+	server.etcdClient = etcdClient
+	ctx, cancel := context.WithCancel(context.Background())
+	server.election = &election.Election{}
+	server.election.SetIsLeader(true)


How about adding a comment to highlight the purpose here?

updated 0201e85

IANTHEREAL · 2019-12-04T03:07:36Z

dm/master/server.go

@@ -253,6 +267,21 @@ func errorCommonWorkerResponse(msg string, worker string) *pb.CommonWorkerRespon

 // StartTask implements MasterServer.StartTask
 func (s *Server) StartTask(ctx context.Context, req *pb.StartTaskRequest) (*pb.StartTaskResponse, error) {
+	log.L().Info("receive request and will save it to etcd", zap.Stringer("payload", req), zap.String("request", "StartTask"))


can we remove will save it to etcd, because we must to save it into etcd

removed f0931fb

IANTHEREAL · 2019-12-04T03:22:05Z

dm/master/server.go

+		}, nil
+	}
+
+	_, leaderID, err := s.election.LeaderInfo(ctx)


what‘s leaderID in DM?

maybe it's better to add a function getDMLeaderMasterInfo

update in 20524e7

IANTHEREAL · 2019-12-04T03:30:35Z

dm/master/server.go

+	for {
+		select {
+		case <-ctx.Done():
+			return


add some log to indicate that the goroutine exited, here or in defer function

add in b091a46

IANTHEREAL · 2019-12-04T03:40:06Z

dm/master/server.go

+
+				// only master leader need handle request
+				if s.election.IsLeader() {
+					go s.handleRequest(string(ev.Kv.Key), operate)


should we manage these goroutines？

I think it is unnecessary, just abandon these requests when leader is exit. dmctl will get error, and user can execute command again later.

What I care about is correctness. If the leader migrates and the transaction processing has not ended, will there be unexpected consequences?

into xiang/etcd_operate_queue

WangXiangUSTC · 2019-12-04T04:12:43Z

/run-all-tests tidb=release-3.0

WangXiangUSTC added 2 commits November 18, 2019 11:44

add command proto and master write some request to etcd, and watch re…

47b099a

…quest in etcd

update some request

b26d68e

WangXiangUSTC added the status/WIP This PR is still work in progress label Nov 18, 2019

WangXiangUSTC added 6 commits November 18, 2019 13:43

update name

6c5c75a

minor update

0143e5f

Merge branch 'master' into xiang/etcd_operate_queue

889e7a9

use etcd in tidb-tools

6709763

fix test

f85e98c

fix unit test

a946031

WangXiangUSTC added 2 commits November 20, 2019 17:16

add some error

62afe6e

minor update

d7298f5

fix

0b0d372

WangXiangUSTC requested review from csuzhangxc and IANTHEREAL November 20, 2019 10:56

csuzhangxc added the type/compatibility Backward compatibility will be broken after this PR merged label Nov 21, 2019

csuzhangxc reviewed Nov 21, 2019

View reviewed changes

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

WangXiangUSTC added 4 commits November 21, 2019 16:44

add Unknown operate type

6b869ef

address comment

9891c9a

update function saveRequestAndWaitResponse

83bc4cb

update function name

b640925

WangXiangUSTC added 3 commits December 2, 2019 17:00

add ShowMasterLeader rpc server, and update check master online

1a82e68

add operate stage

26abd88

merge master

a7759dc

add some log

2e67069

minor fix

3fd9af5

add show master leader test

5f75338

fix ci

39a1fbc

csuzhangxc reviewed Dec 3, 2019

View reviewed changes

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

Merge branch 'master' into xiang/etcd_operate_queue

b8518cd

IANTHEREAL reviewed Dec 4, 2019

View reviewed changes

update log

f0931fb

IANTHEREAL reviewed Dec 4, 2019

View reviewed changes

add comment for unit test

0201e85

IANTHEREAL reviewed Dec 4, 2019

View reviewed changes

WangXiangUSTC added 3 commits December 4, 2019 11:59

update proto and other update

20524e7

Merge branch 'xiang/etcd_operate_queue' of https://github.com/pingcap/dm

7d89605

into xiang/etcd_operate_queue

add log for watch request

b091a46

csuzhangxc mentioned this pull request Dec 6, 2019

Make DM become HA #406

Open

23 tasks

WangXiangUSTC closed this Dec 17, 2019

WangXiangUSTC deleted the xiang/etcd_operate_queue branch January 9, 2020 06:58

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


		defaultOperatePath = "/dm-operate"

		defaultEtcdTimeout = time.Duration(10 * time.Second)

master: use etcd as operate queue #363

master: use etcd as operate queue #363

Conversation

WangXiangUSTC commented Nov 18, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

WangXiangUSTC commented Nov 20, 2019

WangXiangUSTC commented Nov 20, 2019

codecov bot commented Nov 20, 2019 • edited Loading

Codecov Report

WangXiangUSTC commented Nov 20, 2019

WangXiangUSTC commented Nov 20, 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

WangXiangUSTC commented Dec 2, 2019

WangXiangUSTC commented Dec 2, 2019

WangXiangUSTC commented Dec 2, 2019

WangXiangUSTC commented Dec 3, 2019

WangXiangUSTC commented Dec 3, 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

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

WangXiangUSTC commented Dec 4, 2019

WangXiangUSTC commented Nov 18, 2019 •

edited

Loading

codecov bot commented Nov 20, 2019 •

edited

Loading