Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimist: return ConflictNone if a conflict DDL has no conflict with at lease one normal table #5414

Merged
merged 7 commits into from
May 17, 2022
Merged

Optimist: return ConflictNone if a conflict DDL has no conflict with at lease one normal table #5414

merged 7 commits into from
May 17, 2022

Conversation

GMHDBJD
Copy link
Contributor

@GMHDBJD GMHDBJD commented May 13, 2022

What problem does this PR solve?

Issue Number: ref #4287

What is changed and how it works?

When a conflict DDL has no conflict with at least one normal table, we regard it can be executed in downstream.
e.g.

tb1: create table tb(id int, a int)
tb2: create table tb(id int, b int)
tb3: create table tb(id int, b int)

when we receive a conflict ddl alter table tb2 rename b to a, before this pr, tb2 will be skipped and wait tb3's rename column DDL, after this pr, tb2 will directly executed this DDL because tb1 has already have column a and no column b. That is to say we use a more optimist algorithm in this pr.

Check List

Tests

  • Unit test

Release note 

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented May 13, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Ehco1996
  • lichunzhu

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 submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 13, 2022
GMHDBJD
GMHDBJD commented May 13, 2022
View reviewed changes
dm/pkg/shardddl/optimism/lock.go
}

// judge this normal table is smaller(same as allTableSmaller)
if _, err = joined.Compare(prevTable); err == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy from

tiflow/dm/pkg/shardddl/optimism/lock.go

Line 1003 in 08928bf

if _, err = joined.Compare(ti); err == nil {

dm/pkg/shardddl/optimism/lock.go
}

// judge this normal table is larger(same as allTableLarger)
if joined, err = prevTable.Join(ti); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy from

tiflow/dm/pkg/shardddl/optimism/lock.go

Line 1043 in 08928bf

joined, err := ti.Join(finalTi)

dm/pkg/shardddl/optimism/lock.go
@@ -889,8 +889,8 @@ func (l *Lock) trySyncForOneDDL(source, schema, table string, prevTable, postTab

log.L().Info("found conflict for DDL", zap.String("source", source), zap.String("schema", schema), zap.String("table", table), zap.Stringer("prevTable", prevTable), zap.Stringer("postTable", postTable), log.ShortError(tableErr))

if idempotent {
log.L().Info("return conflict DDL for idempotent DDL", zap.String("source", source), zap.String("schema", schema), zap.String("table", table), zap.Stringer("prevTable", prevTable), zap.Stringer("postTable", postTable))
if idempotent || l.noConflictWithOneNormalTable(source, schema, table, prevTable, postTable) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idempotent means the DDL has already been executed in this table before.
noConflictWithOneNormalTable means the DDL has already been executed in at least one other table before.

@GMHDBJD GMHDBJD added the area/dm Issues or PRs related to DM. label May 13, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2022 •
edited
Loading

Codecov Report

Merging #5414 (b395963) into master (687e248) will decrease coverage by 0.4643%.
The diff coverage is 51.9214%.

Flag Coverage Δ
cdc 60.4593% <51.9214%> (-0.1393%) ⬇️
dm 51.9449% <ø> (-0.5247%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@               Coverage Diff                @@
##             master      #5414        +/-   ##
================================================
- Coverage   56.1240%   55.6597%   -0.4644%     
================================================
  Files           522        528         +6     
  Lines         65325      69456      +4131     
================================================
+ Hits          36663      38659      +1996     
- Misses        25094      27095      +2001     
- Partials       3568       3702       +134

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented May 13, 2022

/run-all-tests

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented May 16, 2022

/run-all-tests
cc @lance6716 @lichunzhu

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented May 16, 2022

/run-all-tests

lichunzhu
lichunzhu approved these changes May 16, 2022
View reviewed changes
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 16, 2022
@lichunzhu
Copy link
Contributor

/run-all-tests

Ehco1996
Ehco1996 approved these changes May 17, 2022
View reviewed changes
Copy link
Contributor

@Ehco1996 Ehco1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 17, 2022
@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented May 17, 2022

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: b395963

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 17, 2022
@ti-chi-bot ti-chi-bot merged commit 90e26a2 into pingcap:master May 17, 2022
sdojjy pushed a commit to sdojjy/tiflow that referenced this pull request May 18, 2022
@GMHDBJD @sdojjy
Optimist: return ConflictNone if a conflict DDL has no conflict with …
f0e56aa 
…at lease one normal table (pingcap#5414)

ref pingcap#4287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ehco1996 Ehco1996 Ehco1996 approved these changes

@lichunzhu lichunzhu lichunzhu approved these changes

Assignees
No one assigned
Labels
area/dm Issues or PRs related to DM. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GMHDBJD @ti-chi-bot @codecov-commenter @lichunzhu @Ehco1996