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

syncer: don't clear online ddl if execute ddl failed #449

Merged
merged 18 commits into from
Jan 20, 2020
Merged

syncer: don't clear online ddl if execute ddl failed #449

merged 18 commits into from
Jan 20, 2020

Conversation

WangXiangUSTC
Copy link
Contributor

What problem does this PR solve?

when execute ddl failed, ignore clean online ddl information

What is changed and how it works?

  • check execErrorDetected after add ddl job, if execErrorDetected is true, return error for handleQueryEvent
  • update sync unit's IsFreshJob by the way, if have shard table's checkpoint information, the unit should not be fresh job

Check List

Tests

  • Integration test(will add it later)

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

@WangXiangUSTC WangXiangUSTC added priority/important Major change, requires approval from ≥2 primary reviewers status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix labels Jan 9, 2020
WangXiangUSTC
WangXiangUSTC commented Jan 9, 2020
View reviewed changes
syncer/db.go
@@ -192,6 +193,15 @@ func (conn *DBConn) querySQL(tctx *tcontext.Context, query string, args ...inter
}

func (conn *DBConn) executeSQLWithIgnore(tctx *tcontext.Context, ignoreError func(error) bool, queries []string, args ...[]interface{}) (int, error) {

failpoint.Inject("ExecuteSQLWithIgnoreFailed", func(val failpoint.Value) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will use this failpoint later

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests tidb=release-3.0

@WangXiangUSTC WangXiangUSTC added status/WIP This PR is still work in progress and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jan 9, 2020
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests tidb=release-3.0

WangXiangUSTC
WangXiangUSTC commented Jan 9, 2020
View reviewed changes
syncer/syncer.go
@@ -1054,9 +1055,9 @@ func (s *Syncer) Run(ctx context.Context) (err error) {
s.tctx.L().Error("panic log", zap.Reflect("error message", err1), zap.Stack("statck"))
err = terror.ErrSyncerUnitPanic.Generate(err1)
}
// flush the jobs channels, but if error occurred, we should not flush the checkpoints
if err1 := s.flushJobs(); err1 != 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.

seems that don't need to execute theses sqls, if sync unit exit because of error or user pause the task, will cancel the context, and all sqls will execute failed.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to s.jobWg.Wait() all operation jobs to return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add in 20cfd53

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests tidb=release-3.0

@WangXiangUSTC WangXiangUSTC 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 Jan 9, 2020
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests tidb=release-3.0

@codecov
Copy link

codecov bot commented Jan 9, 2020 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master       #449   +/-   ##
===========================================
  Coverage          ?   57.3182%           
===========================================
  Files             ?        167           
  Lines             ?      17265           
  Branches          ?          0           
===========================================
  Hits              ?       9896           
  Misses            ?       6397           
  Partials          ?        972

@csuzhangxc
Copy link
Member

@csuzhangxc @lichunzhu PTAL

csuzhangxc
csuzhangxc reviewed Jan 12, 2020
View reviewed changes
syncer/checkpoint.go Outdated Show resolved Hide resolved
syncer/db.go Outdated Show resolved Hide resolved
syncer/syncer.go Outdated Show resolved Hide resolved
@csuzhangxc
Copy link
Member

/run-all-tests tidb=release-3.0

@lichunzhu
Copy link
Contributor

/run-all-tests tidb=release-3.0

@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests tidb=release-3.0

1 similar comment
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests tidb=release-3.0

csuzhangxc
csuzhangxc reviewed Jan 19, 2020
View reviewed changes
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc 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 Jan 19, 2020
@csuzhangxc
Copy link
Member

@WangXiangUSTC should we need to add the label of should-cherry-pick-1.0?

@lichunzhu PTAL again.

lichunzhu
lichunzhu approved these changes Jan 19, 2020
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

@lichunzhu lichunzhu added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Jan 19, 2020
@WangXiangUSTC WangXiangUSTC added already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 and removed already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked labels Jan 20, 2020
@WangXiangUSTC
Copy link
Contributor Author

@WangXiangUSTC should we need to add the label of should-cherry-pick-1.0?

@lichunzhu PTAL again.

I add the tag

@WangXiangUSTC WangXiangUSTC merged commit 1433d4a into master Jan 20, 2020
@WangXiangUSTC WangXiangUSTC deleted the xiang/fix_online_ddl branch January 20, 2020 03:28
@sre-bot
Copy link

sre-bot commented Jan 20, 2020

cherry pick to release-1.0 failed

@csuzhangxc
Copy link
Member

@WangXiangUSTC Please cherry-pick this PR manually.

@WangXiangUSTC WangXiangUSTC mentioned this pull request Feb 3, 2020
Merged
@WangXiangUSTC WangXiangUSTC added already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 labels Feb 3, 2020
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
@WangXiangUSTC
syncer: don't clear online ddl if execute ddl failed (pingcap#449)
17b6f3b
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@csuzhangxc csuzhangxc csuzhangxc left review comments

@lichunzhu lichunzhu lichunzhu approved these changes

Assignees
No one assigned
Labels
already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked priority/important Major change, requires approval from ≥2 primary reviewers status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@WangXiangUSTC @csuzhangxc @lichunzhu @sre-bot