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

Optimsitic: refine optimstic create/drop table #4497

Merged
merged 98 commits into from
Mar 17, 2022
Merged

Optimsitic: refine optimstic create/drop table #4497

merged 98 commits into from
Mar 17, 2022

Conversation

GMHDBJD
Copy link
Contributor

@GMHDBJD GMHDBJD commented Jan 27, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: close #3823

What is changed and how it works?

  • worker flush table info, put source tables when create/drop table
  • master watch source tables and add/remove table for lock

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

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 cherry-pick to the release branch
  • Need to update the documentation
  • Need to update key monitor metrics in both TiCDC document and official document

Release note 

`None`.

GMHDBJD and others added 30 commits December 15, 2021 23:27
@GMHDBJD
init logic
cf3881c
@GMHDBJD
remove old code
3022cc5
@GMHDBJD
Merge remote-tracking branch 'upstream/master' into supportInconsiste…
d509b0c 
…ntSchema
@GMHDBJD
remove meta when stop task
25c1d59
@GMHDBJD
Merge remote-tracking branch 'upstream/master' into supportInconsiste…
7b0ad14 
…ntSchema
@GMHDBJD
fix sequence_sharding_optimistic
296fca9
@GMHDBJD
remove old code
f9a691f
@GMHDBJD
fix lint
48b58b7
@GMHDBJD
fix update ba rule test
462cc78
@GMHDBJD
Merge remote-tracking branch 'upstream/master' into supportInconsiste…
7034319 
…ntSchema
@GMHDBJD
fix ut
d61d3fe
@GMHDBJD
Merge remote-tracking branch 'upstream/master' into supportInconsiste…
fabbfd5 
…ntSchema
@GMHDBJD
code review
fdfe741
@GMHDBJD
Merge remote-tracking branch 'upstream/master' into supportInconsiste…
a7cce9b 
…ntSchema
@GMHDBJD
fix lint
6327cbc
@GMHDBJD
add DIFFERENT_SCHEMA_FULL_CASE
b2e89e3
@GMHDBJD
fix lint
c31815d
@GMHDBJD
add test
11a6908
@GMHDBJD
add DIFFERENT_SCHEMA_INCREMENTAL_CASE
d6988fd
@GMHDBJD
add more integration test
9d1921b
@GMHDBJD
Merge remote-tracking branch 'upstream/master' into supportInconsiste…
ff2fa16 
…ntSchema
@GMHDBJD
Merge branch 'master' into supportInconsistentSchema
ac35de9
@GMHDBJD
add unit test
0af4f18
@GMHDBJD
update fmt
7c449ac
@GMHDBJD
Merge remote-tracking branch 'upstream/master' into supportInconsiste…
ee3284f 
…ntSchema
@GMHDBJD
code review
84d9aeb
@GMHDBJD
remove downstream when stop task
9fb1d3a
@GMHDBJD
remove metadata after stop task succeed
00171da
@GMHDBJD
Merge branch 'master' into supportInconsistentSchema
b05c11b
@GMHDBJD
Merge branch 'master' into supportInconsistentSchema
4c3a20e
lichunzhu
lichunzhu reviewed Mar 15, 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.

https://github.com/GMHDBJD/ticdc/blob/refineOptimsticCreateTable/dm/dm/master/shardddl/optimist_test.go#L488
Should we remove this function and use the new method in UTs?
Will review later

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Mar 16, 2022

/run-all-tests

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Mar 17, 2022

cc @lichunzhu

lichunzhu
lichunzhu approved these changes Mar 17, 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 Mar 17, 2022
@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Mar 17, 2022

/run-all-tests

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Mar 17, 2022

/run-all-tests

lance6716
lance6716 reviewed Mar 17, 2022
View reviewed changes
Copy link
Contributor

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

will review later

dm/dm/master/shardddl/optimist.go
for routeTable := range droppedTable {
lock := o.lk.FindLock(utils.GenDDLLockID(st.Task, routeTable.DownSchema, routeTable.DownTable))
if lock != nil {
cols := lock.TryRemoveTable(st.Source, routeTable.UpSchema, routeTable.UpTable)
Copy link
Contributor

Choose a reason for hiding this comment

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

if after remove tables the joined table has changed and a DDL should be executed to downstream, how do we handle it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot handle it. e.g. 2 tables
table1 drop column a
drop table2
Now actually we should drop column a in Downstream, but we can't now.

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Mar 17, 2022

/run-all-tests

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Mar 17, 2022

/run-verify

@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 Mar 17, 2022
@lance6716
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: efa693d

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 17, 2022
@lance6716
Copy link
Contributor

/run-dm-integration-test

@GMHDBJD
Copy link
Contributor Author

GMHDBJD commented Mar 17, 2022

/run-dm-integration-test
/run-verify

@ti-chi-bot ti-chi-bot merged commit 8bbbf7e into pingcap:master Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lance6716 lance6716 lance6716 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/XXL Denotes a PR that changes 1000+ 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.

fatal error: concurrent map read and map write in Optimistic Sharding DDL
5 participants
@GMHDBJD @ti-chi-bot @codecov-commenter @lance6716 @lichunzhu