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

ddl: fix cancel rename index ddl error #8610

Merged
merged 4 commits into from
Jan 8, 2019
Merged

ddl: fix cancel rename index ddl error #8610

merged 4 commits into from
Jan 8, 2019

Conversation

ciscoxll
Copy link
Contributor

@ciscoxll ciscoxll commented Dec 6, 2018 •
edited
Loading

What problem does this PR solve?

Releate PR #8171 , If we cancel rename index ddl job must guarantee correct rollback.

What is changed and how it works?

  • We can only cancel rename index job when the job state is on JobStateNone , If the job status is JobStateDone or JobStateRunning, the job will not be canceled.
  • Only added tests.

Check List

  • Unit test

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

This change is Reviewable

@ciscoxll ciscoxll force-pushed the cancel-rename-index branch from 3f844f0 to 26e34a9 Compare December 10, 2018 03:31
@ciscoxll
Copy link
Contributor Author

@crazycs520 PTAL

@ciscoxll
Copy link
Contributor Author

@crazycs520 PTAL

crazycs520
crazycs520 reviewed Dec 24, 2018
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@ciscoxll ciscoxll added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 25, 2018
@crazycs520
Copy link
Contributor

/run-all-tests

@crazycs520
Copy link
Contributor

@ciscoxll PLease fix ci.

@ciscoxll
Copy link
Contributor Author

@zimulala @winkyao PTAL

1 similar comment
@ciscoxll
Copy link
Contributor Author

ciscoxll commented Jan 4, 2019

@zimulala @winkyao PTAL

@winkyao
Copy link
Contributor

winkyao commented Jan 5, 2019 •
edited
Loading

Please handle the canceling rename index job explicitly in convertJob2RollbackJob

winkyao
winkyao suggested changes Jan 5, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

Please address comment

@ciscoxll ciscoxll force-pushed the cancel-rename-index branch from 562e93c to db3294c Compare January 6, 2019 15:12
@codecov-io
Copy link

codecov-io commented Jan 6, 2019 •
edited
Loading

Codecov Report

Merging #8610 into master will decrease coverage by <.01%.
The diff coverage is 51.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8610      +/-   ##
==========================================
- Coverage   67.54%   67.54%   -0.01%     
==========================================
  Files         363      363              
  Lines       75247    75264      +17     
==========================================
+ Hits        50827    50835       +8     
- Misses      19933    19939       +6     
- Partials     4487     4490       +3
Impacted Files Coverage Δ
ddl/index.go 79.62% <41.17%> (ø) ⬆️
ddl/rollingback.go 70.42% <66.66%> (-0.35%) ⬇️
util/systimemon/systime_mon.go 80% <0%> (-20%) ⬇️
expression/schema.go 94.11% <0%> (-0.85%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddda6b4...ca8e2cd. Read the comment docs.

winkyao
winkyao reviewed Jan 7, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao
Copy link
Contributor

winkyao commented Jan 7, 2019

@zimulala PTAL

@winkyao winkyao added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 7, 2019
zimulala
zimulala reviewed Jan 8, 2019
View reviewed changes
zimulala
zimulala reviewed Jan 8, 2019
View reviewed changes
winkyao
winkyao previously approved these changes Jan 8, 2019
View reviewed changes
zimulala
zimulala approved these changes Jan 8, 2019
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala
Copy link
Contributor

zimulala commented Jan 8, 2019

/run-all-tests

@zimulala zimulala added the status/LGT3 The PR has already had 3 LGTM. label Jan 8, 2019
@zimulala zimulala removed the status/LGT2 Indicates that a PR has LGTM 2. label Jan 8, 2019
@ciscoxll
Copy link
Contributor Author

ciscoxll commented Jan 8, 2019

/run-all-tests

@ciscoxll ciscoxll merged commit da31a46 into pingcap:master Jan 8, 2019
@ciscoxll ciscoxll deleted the cancel-rename-index branch January 8, 2019 11:29
zhexuany pushed a commit to zhexuany/tidb that referenced this pull request Jan 10, 2019
@ciscoxll @zhexuany
ddl: fix cancel rename index ddl error (pingcap#8610)
f934cef
@ciscoxll ciscoxll mentioned this pull request Feb 20, 2019
Merged
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazycs520 crazycs520 crazycs520 left review comments

@zimulala zimulala zimulala approved these changes

@winkyao winkyao winkyao left review comments

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ciscoxll @crazycs520 @winkyao @codecov-io @zimulala @you06 @sre-bot