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

syncer: fast fail on some error in ddl execution #317

Merged
merged 7 commits into from
Oct 17, 2019

Conversation

amyangfei
Copy link
Contributor

What problem does this PR solve?

syncer doesn't need to retry on the unsupported DDL execution error, such as can't drop column with index, unsupported drop integer primary key, etc. Pause task directly with error is enough when meeting these errors.

What is changed and how it works?

add error blacklist in db operation retryable decision

Check List

Tests

  • Unit test
  • Integration test

Code changes

Side effects

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note
    Fast fail on some TiDB unsupported DDL error in the binlog replication unit.

@amyangfei amyangfei added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/enhancement Performance improvement or refactoring 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 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Oct 17, 2019
@codecov
Copy link

codecov bot commented Oct 17, 2019

Codecov Report

Merging #317 into master will decrease coverage by 0.1614%.
The diff coverage is 27.2727%.

@@               Coverage Diff                @@
##             master       #317        +/-   ##
================================================
- Coverage   60.7287%   60.5672%   -0.1615%     
================================================
  Files           134        134                
  Lines         14848      14843         -5     
================================================
- Hits           9017       8990        -27     
- Misses         4962       4982        +20     
- Partials        869        871         +2

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.

rest LGTM

syncer/db.go Show resolved Hide resolved
pkg/retry/errors.go Outdated Show resolved Hide resolved
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 Oct 17, 2019
@csuzhangxc
Copy link
Member

@WangXiangUSTC PTAL

WangXiangUSTC
WangXiangUSTC previously approved these changes Oct 17, 2019
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Oct 17, 2019
@amyangfei amyangfei merged commit 3da0435 into pingcap:master Oct 17, 2019
@amyangfei amyangfei deleted the db-error-fast-fail branch October 17, 2019 07:30
@sre-bot
Copy link

sre-bot commented Oct 17, 2019

cherry pick to release-1.0 failed

@amyangfei amyangfei 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 Oct 18, 2019
@csuzhangxc csuzhangxc added already-update-release-note The release note is updated. Add this label once the release note is updated and removed needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Nov 1, 2019
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 already-update-release-note The release note is updated. Add this label once the release note is updated priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/enhancement Performance improvement or refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants