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

syncer: fix ignore canceled error #401

Merged
merged 1 commit into from
Dec 5, 2019

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Dec 5, 2019

What problem does this PR solve?

in #355, ignore canceled errors for executing SQLs is introduced.

but miss to ignore the canceled errors for executing DMLs.

What is changed and how it works?

check !utils.IsContextCanceledError(err) before s.runFatalChan <- unit.NewProcessError(errType, err) for executing DMLs as we did for DDL.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    • hack the code for executing DMLs and return an error in one goroutine
    • obverse the log and result of query-status for context canceled error

Related changes

@csuzhangxc csuzhangxc 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/bug-fix Bug fix 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 Dec 5, 2019
@csuzhangxc csuzhangxc added this to the v1.0.3 milestone Dec 5, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

@WangXiangUSTC @amyangfei PTAL

@codecov
Copy link

codecov bot commented Dec 5, 2019

Codecov Report

Merging #401 into master will increase coverage by 0.1258%.
The diff coverage is 0%.

@@               Coverage Diff                @@
##             master       #401        +/-   ##
================================================
+ Coverage   57.5617%   57.6876%   +0.1258%     
================================================
  Files           163        163                
  Lines         16471      16442        -29     
================================================
+ Hits           9481       9485         +4     
+ Misses         6065       6036        -29     
+ Partials        925        921         -4

@WangXiangUSTC
Copy link
Contributor

LGTM

@WangXiangUSTC WangXiangUSTC 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 Dec 5, 2019
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

LGTM

@amyangfei amyangfei added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Dec 5, 2019
@csuzhangxc csuzhangxc merged commit 9a07a50 into pingcap:master Dec 5, 2019
@csuzhangxc csuzhangxc deleted the fix-ignore-cancel branch December 5, 2019 06:41
@sre-bot
Copy link

sre-bot commented Dec 5, 2019

cherry pick to release-1.0 in PR #402

@sre-bot sre-bot 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 Dec 5, 2019
@csuzhangxc csuzhangxc removed the needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated label Mar 13, 2020
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 priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants