pessimistic: record latest done ddl to handle conflict #1626

GMHDBJD · 2021-04-23T08:28:47Z

What problem does this PR solve?

What is changed and how it works?

when latest DDLs is resolved, put it into etcd.
when conflict DDLs detected in master, ignore DDLs which equals to the latest DDLs.

Check List

Tests

Unit test
Integration test

ti-chi-bot · 2021-04-23T08:28:48Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

lance6716 · 2021-04-24T14:18:28Z

could we let DM-worker put ShardDDLPessimismDDLsKeyAdapter, to handle DM-master down between "receive the done lock through etcd" and "put ShardDDLPessimismDDLsKeyAdapter"?
And putting ShardDDLPessimismDDLsKeyAdapter should be in the same transaction with marking the lock done.

And I don't know if there're some DDL that could be run twice with some DML interleaved and take some effects (like TRUNCATE but TRUNCATE is filtered, I don't know if we should support pessimistic sharding TRUNCATE someday). For this case, we might adding binlog location as a part of key of ShardDDLPessimismDDLsKeyAdapter

GMHDBJD · 2021-04-25T05:46:17Z

could we let DM-worker put ShardDDLPessimismDDLsKeyAdapter, to handle DM-master down between "receive the done lock through etcd" and "put ShardDDLPessimismDDLsKeyAdapter"?

So you mean we should record last done ddl for every worker? Currently only put last done ddl after lock is resolved(all source done). Here handle the situation about DM-master down.

And I don't know if there're some DDL that could be run twice with some DML interleaved and take some effects

I don't think we will support truncate in pessimistic.🤣. We only skip the DDL when conflict error happened. So the case only happened like

source1 ddl1
source2 ddl1
done
source1 ddl1 <- skip
source2 ddl2

we handle this situation.

dm/common/common.go

lichunzhu · 2021-04-25T12:07:52Z

pkg/shardddl/pessimism/operation.go


 	// only used to report to the caller of the watcher, do not marsh it.
 	// if it's true, it means the Operation has been deleted in etcd.
 	IsDeleted bool `json:"-"`
 }

 // NewOperation creates a new Operation instance.
-func NewOperation(id, task, source string, ddls []string, exec, done bool) Operation {
+func NewOperation(id, task, source string, ddls []string, exec, done bool, skip bool) Operation {


Suggested change

func NewOperation(id, task, source string, ddls []string, exec, done bool, skip bool) Operation {

func NewOperation(id, task, source string, ddls []string, exec, done, skip bool) Operation {

lichunzhu · 2021-04-25T12:10:35Z

pkg/shardddl/pessimism/lock.go

@@ -64,13 +68,46 @@ func NewLock(id, task, owner string, ddls, sources []string) *Lock {
 // TrySync tries to sync the lock, does decrease on remain, re-entrant.
 // new upstream sources may join when the DDL lock is in syncing,
 // so we need to merge these new sources.
-func (l *Lock) TrySync(caller string, ddls, sources []string) (bool, int, error) {
+func (l *Lock) TrySync(cli *clientv3.Client, caller string, ddls, sources []string, latestDoneDDLs []string) (bool, int, error) {


Suggested change

func (l *Lock) TrySync(cli *clientv3.Client, caller string, ddls, sources []string, latestDoneDDLs []string) (bool, int, error) {

func (l *Lock) TrySync(cli *clientv3.Client, caller string, ddls, sources, latestDoneDDLs []string) (bool, int, error) {

lichunzhu · 2021-04-25T12:18:34Z

pkg/shardddl/pessimism/lock.go

+		}
+
+		// current ddls idempotent, skip it.
+		if utils.CompareShardingDDLs(latestDoneDDLs, ddls) {


We can save this function in a bool variable to save one comparison.

lance6716

will review later

dm/master/shardddl/pessimist.go

lance6716 · 2021-04-26T02:43:27Z

pkg/shardddl/pessimism/ddls.go

+}
+
+// PutLatestDoneDDLs puts the last done shard DDL ddls into etcd.
+func PutLatestDoneDDLs(cli *clientv3.Client, lockID string, ddls []string) (int64, error) {


may use above function

lance6716 · 2021-04-26T02:48:37Z

pkg/shardddl/pessimism/keeper.go

+		if t := utils.ExtractTaskFromLockID(lockID); t == task {
+			lockIDs = append(lockIDs, lockID)
+		}
+		delete(lk.latestDoneDDLs, lockID)


should we remove the lockID only when it matching the task 🤔

lance6716 · 2021-04-26T02:52:44Z

pkg/shardddl/pessimism/operation.go

@@ -35,21 +35,23 @@ type Operation struct {
 	DDLs   []string `json:"ddls"`   // DDL statements
 	Exec   bool     `json:"exec"`   // execute or skip the DDL statements
 	Done   bool     `json:"done"`   // whether the `Exec` operation has done
+	Skip   bool     `json:"skip"`   // Whether worker skip this operation


could we don't add Skip, instead, use empty DDLs

syncer/syncer.go

pkg/shardddl/pessimism/keeper.go

lance6716 · 2021-05-18T03:06:46Z

need shfmt

lance6716

will review later

pkg/utils/common.go

dm/master/shardddl/pessimist.go

lance6716 · 2021-05-18T04:12:57Z

pkg/shardddl/pessimism/lock.go

+			_, _, err := PutOperations(cli, true, NewOperation(l.ID, l.Task, caller, latestDoneDDLs, false, false, true))
+			return l.remain <= 0, l.remain, err
+		}
+


could you give an example of the cases that enter here 😵 ?

curIdempotent: worker1, worker2 ddl1 -> worker 2 ddl2 -> worker1 resync ddl1
otherIdempotent: worker1, worker2 ddl1 -> worker2 resync ddl1 -> worker1 ddl2

pkg/shardddl/pessimism/ops.go

lance6716 · 2021-05-19T04:24:52Z

pkg/shardddl/pessimism/lock.go

+		}
+
+		// current ddls idempotent, skip it.
+		if curIdempotent {


I still prefer that we should check curIdempotent before utils.CompareShardingDDLs(ddls, l.DDLs)

background: the lock is resolved but one non-owner is failed before saving checkpoint and resync the DDL of the resolved lock

my prefer way:

the failed DM-worker put the outdated DDL. if we check curIdempotent here, we could earlier avoid blocking that DM-worker which may caused by DDL lock

current way:

the failed DM-worker (worker1) put the outdated DDL, utils.CompareShardingDDLs(ddls, l.DDLs) is true

another DM-worker (worker2) put a DDL more likely a new different DDL, enter if-then branch, curIdempotent is false, utils.CompareShardingDDLs(latestDoneDDLs, l.DDLs) is true.
(And i think in most cases curIdempotent is false. And when curIdempotent is true, worker1 will wait more time to get skip Op)

in step2 we enter "other sources' ddls idempotent, skip them" branch, and here we put skip Op for worker1

Above all, worker1 may be blocked for a longer time

And I still prefer save a binlog position through same transaction in etcd 😂 When DM-worker starting, this position has higher priority than one in downstream checkpoint

GMHDBJD · 2021-05-19T05:02:38Z

/hold

ti-chi-bot · 2021-06-04T07:48:54Z

@GMHDBJD: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lance6716 · 2021-08-17T01:38:02Z

/hold

pessimistic: record latest done ddl to handle conflict

69f56d4

ti-chi-bot requested a review from lichunzhu April 23, 2021 08:28

ti-chi-bot added the size/XL label Apr 23, 2021

GMHDBJD added status/WIP This PR is still work in progress type/bug-fix Bug fix needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 labels Apr 23, 2021

GMHDBJD added 2 commits April 23, 2021 16:49

add ut

2460943

fix test

fc5f898

GMHDBJD 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 Apr 23, 2021

lance6716 modified the milestone: v2.0.3 Apr 25, 2021

lance6716 added the needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated label Apr 25, 2021

lance6716 added this to the v2.0.3 milestone Apr 25, 2021

atomic put latest done ddls

c9c9b49

lichunzhu reviewed Apr 25, 2021

View reviewed changes

lance6716 reviewed Apr 26, 2021

View reviewed changes

ti-chi-bot added the needs-rebase label Apr 26, 2021

GMHDBJD added 5 commits May 6, 2021 10:09

Merge remote-tracking branch 'upstream/master' into fixPessmistic

fd1ae14

Merge remote-tracking branch 'upstream/master' into fixPessmistic

7ed9987

Merge remote-tracking branch 'upstream/master' into fixPessmistic

37f9211

Merge remote-tracking branch 'upstream/master' into fixPessmistic

73f49f4

address comment

a770b5d

ti-chi-bot removed the needs-rebase label May 7, 2021

lance6716 modified the milestones: v2.0.3, v2.0.4 May 8, 2021

lance6716 reviewed May 12, 2021

View reviewed changes

pkg/shardddl/pessimism/keeper.go Show resolved Hide resolved

GMHDBJD added 2 commits May 12, 2021 13:07

Merge branch 'master' into fixPessmistic

cb84241

Merge branch 'master' into fixPessmistic

efe5ca0

ti-chi-bot added the needs-rebase label May 13, 2021

Merge remote-tracking branch 'upstream/master' into fixPessmistic

385f8b1

ti-chi-bot removed the needs-rebase label May 18, 2021

fix shfmt

c4405c3

lance6716 reviewed May 18, 2021

View reviewed changes

pkg/utils/common.go Show resolved Hide resolved

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

dm/master/shardddl/pessimist.go Show resolved Hide resolved

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

lance6716 reviewed May 18, 2021

View reviewed changes

GMHDBJD and others added 2 commits May 18, 2021 14:11

address comment

672a5b7

Merge branch 'master' into fixPessmistic

956d9d3

lance6716 reviewed May 19, 2021

View reviewed changes

ti-chi-bot added the do-not-merge/hold label May 19, 2021

ti-chi-bot added the needs-rebase label Jun 4, 2021

GMHDBJD removed this from the v2.0.4 milestone Jun 7, 2021

Ehco1996 removed the status/PTAL This PR is ready for review. Add this label back after committing new changes label Jun 28, 2021

lance6716 added status/Stale do-not-merge/hold and removed do-not-merge/hold labels Aug 2, 2021

ti-chi-bot added the do-not-merge/hold label Aug 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pessimistic: record latest done ddl to handle conflict #1626

pessimistic: record latest done ddl to handle conflict #1626

GMHDBJD commented Apr 23, 2021

ti-chi-bot commented Apr 23, 2021

lance6716 commented Apr 24, 2021 •

edited

Loading

GMHDBJD commented Apr 25, 2021

lichunzhu Apr 25, 2021

lichunzhu Apr 25, 2021

lichunzhu Apr 25, 2021

lance6716 left a comment

lance6716 Apr 26, 2021

lance6716 Apr 26, 2021

lance6716 Apr 26, 2021

lance6716 commented May 18, 2021

lance6716 left a comment

lance6716 May 18, 2021

GMHDBJD May 18, 2021

lance6716 May 19, 2021

lance6716 May 19, 2021

GMHDBJD commented May 19, 2021

ti-chi-bot commented Jun 4, 2021

lance6716 commented Aug 17, 2021

	func NewOperation(id, task, source string, ddls []string, exec, done bool, skip bool) Operation {
	func NewOperation(id, task, source string, ddls []string, exec, done, skip bool) Operation {

	func (l Lock) TrySync(cli clientv3.Client, caller string, ddls, sources []string, latestDoneDDLs []string) (bool, int, error) {
	func (l Lock) TrySync(cli clientv3.Client, caller string, ddls, sources, latestDoneDDLs []string) (bool, int, error) {

pessimistic: record latest done ddl to handle conflict #1626

Are you sure you want to change the base?

pessimistic: record latest done ddl to handle conflict #1626

Conversation

GMHDBJD commented Apr 23, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

ti-chi-bot commented Apr 23, 2021

lance6716 commented Apr 24, 2021 • edited Loading

GMHDBJD commented Apr 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 left a 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

lance6716 commented May 18, 2021

lance6716 left a 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

GMHDBJD commented May 19, 2021

ti-chi-bot commented Jun 4, 2021

lance6716 commented Aug 17, 2021

lance6716 commented Apr 24, 2021 •

edited

Loading