Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

operate-schema: flush schema info and sync to master in optimistic mode #1506

Merged
merged 20 commits into from
Mar 18, 2021

Conversation

GMHDBJD
Copy link
Collaborator

@GMHDBJD GMHDBJD commented Mar 12, 2021

What problem does this PR solve?

handle rename column statement

What is changed and how it works?

  • add --flush and --sync for operate-schema set
  • flush checkpoint and table info
  • sync table info to master and ignore conflict in optimistic mode

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change

@GMHDBJD GMHDBJD added status/WIP This PR is still work in progress needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated 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 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 12, 2021
@lance6716 lance6716 added this to the v2.0.2 milestone Mar 12, 2021
dm/ctl/master/operate_schema.go Outdated Show resolved Hide resolved
syncer/checkpoint.go Outdated Show resolved Hide resolved
args = append(args, arg)

// use a new context apart from syncer, to make sure when syncer call `cancel` checkpoint could update
tctx2, cancel := tctx.WithContext(context.Background()).WithTimeout(maxDMLConnectionDuration)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suddently realized if this action cost a long time, other checkpoint method maybe blocked. Hope it will not block upward to some user interface commands

Copy link
Collaborator Author

@GMHDBJD GMHDBJD Mar 12, 2021

Choose a reason for hiding this comment

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

Since operate-schema can only use when task paused, the impact of other checkpoint method should be minimal. For user interface, maybe we can use smaller timeout arg?

@lance6716 lance6716 added the needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated label Mar 15, 2021
tests/shardddl1/run.sh Show resolved Hide resolved
syncer/schema.go Outdated Show resolved Hide resolved
syncer/schema.go Show resolved Hide resolved
GMHDBJD and others added 2 commits March 15, 2021 13:33
Co-authored-by: lance6716 <lance6716@gmail.com>
@lance6716
Copy link
Collaborator

/lgtm

@ti-srebot ti-srebot added the status/LGT1 One reviewer already commented LGTM label Mar 15, 2021
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.

rest LGTM

@@ -499,7 +499,9 @@ func (o *Optimist) handleOperationPut(ctx context.Context, opCh <-chan optimism.
func (o *Optimist) handleLock(info optimism.Info, tts []optimism.TargetTable, skipDone bool) error {
lockID, newDDLs, err := o.lk.TrySync(info, tts)
var cfStage = optimism.ConflictNone
if err != nil {
if info.IgnoreConflict {
o.logger.Warn("handle lock in ignore conflict mode", zap.String("lock", lockID), zap.Stringer("info", info))
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if we are in ignore-conflict mode, I think it's better to log the error rather than omit it.

Comment on lines 1660 to 1661
info = NewInfo(task, source, db, tbls[0], downSchema, downTable, DDLs1, nil, []*model.TableInfo{ti1})
info.Version = vers[source][db][tbls[0]]
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add a function NewInfoWithVersion in this test.go since it's used so many times.

syncer/checkpoint.go Outdated Show resolved Hide resolved
@@ -553,6 +557,43 @@ func (cp *RemoteCheckPoint) FlushPointsExcept(tctx *tcontext.Context, exceptTabl
return nil
}

// FlushPointWithTableInfo implements CheckPoint.FlushPointWithTableInfo
func (cp *RemoteCheckPoint) FlushPointWithTableInfo(tctx *tcontext.Context, schema string, table string, ti *model.TableInfo) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right?

Suggested change
func (cp *RemoteCheckPoint) FlushPointWithTableInfo(tctx *tcontext.Context, schema string, table string, ti *model.TableInfo) error {
func (cp *RemoteCheckPoint) FlushPointWithTableInfo(tctx *tcontext.Context, sourceSchema, sourceTable string, ti *model.TableInfo) error {

syncer/checkpoint.go Outdated Show resolved Hide resolved
GMHDBJD and others added 5 commits March 17, 2021 17:01
Co-authored-by: Chunzhu Li <lichunzhu@stu.xjtu.edu.cn>
Co-authored-by: Chunzhu Li <lichunzhu@stu.xjtu.edu.cn>
GMHDBJD and others added 2 commits March 17, 2021 19:20
Co-authored-by: Chunzhu Li <lichunzhu@stu.xjtu.edu.cn>
@lichunzhu lichunzhu merged commit 976688a into pingcap:master Mar 18, 2021
ti-srebot pushed a commit to ti-srebot/dm that referenced this pull request Mar 18, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link

cherry pick to release-2.0 in PR #1514

@ti-srebot ti-srebot added already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked and removed 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 Mar 18, 2021
lance6716 pushed a commit that referenced this pull request Mar 19, 2021
@lance6716 lance6716 removed the needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated label May 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated status/LGT1 One reviewer already commented LGTM status/PTAL This PR is ready for review. Add this label back after committing new changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants