HA: refactor unlock-ddl-lock and break-ddl-lock #522

csuzhangxc · 2020-03-06T09:52:04Z

What problem does this PR solve?

refactor unlock-ddl-lock and break-ddl-lock to support HA.

What is changed and how it works?

re-implement the unlock-ddl-lock based on the etcd model.
remove break-ddl-lock because it's not needed anymore in the new model.

Check List

Tests

Unit test
Integration test

Code changes

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

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

csuzhangxc · 2020-03-06T09:52:18Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2020-03-06T13:51:12Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2020-03-07T03:35:10Z

/run-all-tests tidb=release-3.0

codecov · 2020-03-07T03:47:38Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a711a1c). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             master      #522   +/-   ##
==========================================
  Coverage          ?   56.846%           
==========================================
  Files             ?       183           
  Lines             ?     18821           
  Branches          ?         0           
==========================================
  Hits              ?     10699           
  Misses            ?      7046           
  Partials          ?      1076

csuzhangxc · 2020-03-07T03:48:14Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2020-03-07T04:12:41Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2020-03-07T04:26:10Z

@lichunzhu @WangXiangUSTC PTAL

dm/ctl/master/unlock_ddl_lock.go

dm/master/shardddl/pessimist.go

dm/master/shardddl/pessimist_test.go

…-lock

Co-Authored-By: Chunzhu Li <lichunzhu@pingcap.com>

csuzhangxc · 2020-03-09T10:23:55Z

/run-all-tests tidb=release-3.0

lichunzhu

In some other places:
https://github.com/pingcap/dm/blob/master/pkg/shardddl/pessimism/operation_test.go#L68
https://github.com/pingcap/dm/blob/master/pkg/shardddl/pessimism/operation_test.go#L84
https://github.com/pingcap/dm/blob/master/syncer/shardddl/pessimist.go#L86
We are still using rev instead of rev+1. Should we unify them?
Rest LGTM

…ic-lock

csuzhangxc · 2020-03-09T12:46:28Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2020-03-09T12:49:01Z

In some other places:
https://github.com/pingcap/dm/blob/master/pkg/shardddl/pessimism/operation_test.go#L68
https://github.com/pingcap/dm/blob/master/pkg/shardddl/pessimism/operation_test.go#L84
https://github.com/pingcap/dm/blob/master/syncer/shardddl/pessimist.go#L86
We are still using rev instead of rev+1. Should we unify them?
Rest LGTM

for

> https://github.com/pingcap/dm/blob/master/pkg/shardddl/pessimism/operation_test.go#L68
> https://github.com/pingcap/dm/blob/master/pkg/shardddl/pessimism/operation_test.go#L84

WatchOperationPut is watching with the revision returned from PutOperations, so rev should be used rather than rev+1.

for

> https://github.com/pingcap/dm/blob/master/syncer/shardddl/pessimist.go#L86

I updated in the caller (syncer.go) in 1ddb439.

lichunzhu

LGTM

csuzhangxc · 2020-03-09T15:05:36Z

/run-all-tests tidb=release-3.0

WangXiangUSTC · 2020-03-09T15:25:43Z

pkg/shardddl/pessimism/lock.go

+
+	for _, source := range sources {
+		if synced, ok := l.ready[source]; ok && synced {
+			l.ready[source] = false


do we need to revert l.done[source]

The only usage of RevertSynced is to revert Synced for ForceSynced.
and currenly we can't RevertSynced is the owner done the exec operation (done happened after that).

csuzhangxc · 2020-03-09T15:46:30Z

/run-all-tests tidb=release-3.0

WangXiangUSTC · 2020-03-09T15:48:29Z

dm/master/shardddl/pessimist.go

+// UnlockLock unlocks a shard DDL lock manually when using `unlock-ddl-lock` command.
+// ID: the shard DDL lock ID.
+// replaceOwner: the new owner used to replace the original DDL for executing DDL to downstream.
+//   if the original owner is still exist, we should NOT specify any replaceOwner.


in which case the original owner is not existing?

and maybe we should choose a replaceOwner automatic later.

in which case the original owner is not existing

operate-source stop takes on the original owner.

and maybe we should choose a replaceOwner automatic later.

YES, it's better to do that automatically.

csuzhangxc · 2020-03-09T15:49:51Z

/run-all-tests tidb=release-3.0

WangXiangUSTC · 2020-03-09T16:01:58Z

dm/master/election.go

@@ -61,6 +62,11 @@ func (s *Server) electionNotify(ctx context.Context) {
 					log.L().Error("scheduler do not started", zap.Error(err))
 				}

+				err = s.pessimist.Start(ctx, s.etcdClient)
+				if err != nil {
+					log.L().Error("pessimist do not started", zap.Error(err))


if start pessimist failed, do we need close scheduler and skip set leader in L71?

I think we need a mechanism to transfer leader role to another DM-master member when any necessary components (pessimist and scheduler in currenlty) started fail later.

and close the pessimist

I think we need a mechanism to transfer leader role to another DM-master member when any necessary components (pessimist and scheduler in currenlty) started fail later.

ok, we can do this later

@lichunzhu you can open a issue to trace leader transfer support later.

WangXiangUSTC

rest LGTM

WangXiangUSTC

LGTM

csuzhangxc added 2 commits March 5, 2020 20:40

*: delete shard DDL info only when it exist

75418cd

*: base implement of unlock-ddl-lock

83877b6

csuzhangxc added priority/normal Minor change, requires approval from ≥1 primary reviewer status/WIP This PR is still work in progress type/enhancement Performance improvement or refactoring labels Mar 6, 2020

csuzhangxc added 3 commits March 6, 2020 21:19

*: forceRemove support; add more test case

c939b6c

shardddl: add test case for replace owner.

e815784

*: use the new unlock-ddl-lock implement

a94a408

shardddl: fix support for re-entrant of source; add test case

b54d2bb

*: remove break-ddl-lock because it not needed now.

be86bd9

tests: fix dmctl_command

a421297

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 Mar 7, 2020

csuzhangxc requested review from lichunzhu and WangXiangUSTC March 7, 2020 04:25

csuzhangxc mentioned this pull request Mar 9, 2020

[WIP] Add optimistic sharding mode #491

Closed

lichunzhu reviewed Mar 9, 2020

View reviewed changes

dm/master/shardddl/pessimist_test.go Outdated Show resolved Hide resolved

csuzhangxc and others added 3 commits March 9, 2020 18:21

shardddl: address comments

03479c5

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

642050c

…-lock

Update dm/ctl/master/unlock_ddl_lock.go

ba64562

Co-Authored-By: Chunzhu Li <lichunzhu@pingcap.com>

lichunzhu reviewed Mar 9, 2020

View reviewed changes

csuzhangxc added 2 commits March 9, 2020 20:45

syncer: address comments

1ddb439

Merge remote-tracking branch 'origin/pessimistic-lock' into pessimist…

95f24aa

…ic-lock

lichunzhu reviewed Mar 9, 2020

View reviewed changes

lichunzhu 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 Mar 9, 2020

Merge branch 'master' into pessimistic-lock

659648e

WangXiangUSTC reviewed Mar 9, 2020

View reviewed changes

Merge branch 'master' into pessimistic-lock

0ca40eb

WangXiangUSTC reviewed Mar 9, 2020

View reviewed changes

pessimism: fix merge

bdab79e

WangXiangUSTC reviewed Mar 9, 2020

View reviewed changes

WangXiangUSTC approved these changes Mar 9, 2020

View reviewed changes

csuzhangxc merged commit 263b86b into pingcap:master Mar 9, 2020

csuzhangxc deleted the pessimistic-lock branch March 9, 2020 16:06

lichunzhu mentioned this pull request Mar 9, 2020

Add a mechanism to transfer leader role when any necessary component fail to start #527

Closed

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

HA: refactor unlock-ddl-lock and break-ddl-lock (pingcap#522)

d1ad6cf

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

HA: refactor unlock-ddl-lock and break-ddl-lock (pingcap#522)

6f22737

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

HA: refactor unlock-ddl-lock and break-ddl-lock (pingcap#522)

5f2e34b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HA: refactor unlock-ddl-lock and break-ddl-lock #522

HA: refactor unlock-ddl-lock and break-ddl-lock #522

csuzhangxc commented Mar 6, 2020 •

edited

Loading

csuzhangxc commented Mar 6, 2020

csuzhangxc commented Mar 6, 2020

csuzhangxc commented Mar 7, 2020

codecov bot commented Mar 7, 2020 •

edited

Loading

csuzhangxc commented Mar 7, 2020

csuzhangxc commented Mar 7, 2020

csuzhangxc commented Mar 7, 2020

csuzhangxc commented Mar 9, 2020

lichunzhu left a comment

csuzhangxc commented Mar 9, 2020

csuzhangxc commented Mar 9, 2020

lichunzhu left a comment

csuzhangxc commented Mar 9, 2020

WangXiangUSTC Mar 9, 2020

csuzhangxc Mar 9, 2020

csuzhangxc commented Mar 9, 2020

WangXiangUSTC Mar 9, 2020

WangXiangUSTC Mar 9, 2020

csuzhangxc Mar 9, 2020 •

edited

Loading

csuzhangxc commented Mar 9, 2020

WangXiangUSTC Mar 9, 2020

csuzhangxc Mar 9, 2020 •

edited

Loading

WangXiangUSTC Mar 9, 2020

WangXiangUSTC Mar 9, 2020

csuzhangxc Mar 9, 2020

lichunzhu Mar 9, 2020

WangXiangUSTC left a comment

WangXiangUSTC left a comment

HA: refactor unlock-ddl-lock and break-ddl-lock #522

HA: refactor unlock-ddl-lock and break-ddl-lock #522

Conversation

csuzhangxc commented Mar 6, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

csuzhangxc commented Mar 6, 2020

csuzhangxc commented Mar 6, 2020

csuzhangxc commented Mar 7, 2020

codecov bot commented Mar 7, 2020 • edited Loading

Codecov Report

csuzhangxc commented Mar 7, 2020

csuzhangxc commented Mar 7, 2020

csuzhangxc commented Mar 7, 2020

csuzhangxc commented Mar 9, 2020

lichunzhu left a comment

Choose a reason for hiding this comment

csuzhangxc commented Mar 9, 2020

csuzhangxc commented Mar 9, 2020

lichunzhu left a comment

Choose a reason for hiding this comment

csuzhangxc commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csuzhangxc commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csuzhangxc Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

csuzhangxc commented Mar 9, 2020

Choose a reason for hiding this comment

csuzhangxc Mar 9, 2020 • 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

WangXiangUSTC left a comment

Choose a reason for hiding this comment

WangXiangUSTC left a comment

Choose a reason for hiding this comment

csuzhangxc commented Mar 6, 2020 •

edited

Loading

codecov bot commented Mar 7, 2020 •

edited

Loading

csuzhangxc Mar 9, 2020 •

edited

Loading

csuzhangxc Mar 9, 2020 •

edited

Loading