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.

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

ddl: fix bug of updating tiflash replica status ddl job been stuck. #15001

Merged
merged 13 commits into from
Mar 5, 2020

Conversation

crazycs520
Copy link
Contributor

Signed-off-by: crazycs crazycs520@gmail.com

What problem does this PR solve?

Need cancel DDL job when the update flash replica DDL job need to do nothing.

What is changed and how it works?

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to update the tidb-ansible repository

Release note

  • Fix issue that updates tiflash replica status ddl job been stuck.

Signed-off-by: crazycs <crazycs520@gmail.com>
@crazycs520
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Feb 28, 2020

Codecov Report

Merging #15001 into master will not change coverage by %.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #15001   +/-   ##
===========================================
  Coverage   80.2220%   80.2220%           
===========================================
  Files           503        503           
  Lines        132410     132410           
===========================================
  Hits         106222     106222           
  Misses        17814      17814           
  Partials       8374       8374           

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

Rest is ok

@@ -748,7 +748,8 @@ func onUpdateFlashReplicaStatus(t *meta.Meta, job *model.Job) (ver int64, _ erro
}
if tblInfo.TiFlashReplica == nil || (tblInfo.ID == physicalID && tblInfo.TiFlashReplica.Available == available) ||
(tblInfo.ID != physicalID && available == tblInfo.TiFlashReplica.IsPartitionAvailable(physicalID)) {
Copy link
Contributor

@AilinKid AilinKid Feb 28, 2020

Choose a reason for hiding this comment

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

I think it will return nil in UpdateTableReplicaInfo in ddl_api.go rather than return error in onUpdateFlashReplicaStatus at ddl_woker.go.
you mean for the jobs already in queue? do the double check?
Then, may be you should call the onUpdateFlashReplicaStatus directly.

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

LGTM

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

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-all-tests

@zimulala zimulala added sig/sql-infra SIG: SQL Infra and removed component/DDL1 labels Mar 4, 2020
@zimulala zimulala added the status/can-merge Indicates a PR has been approved by a committer. label Mar 4, 2020
@crazycs520
Copy link
Contributor Author

/run-unit-test

@CLAassistant
Copy link

CLAassistant commented Mar 5, 2020

CLA assistant check
All committers have signed the CLA.

@lidezhu
Copy link
Contributor

lidezhu commented Mar 5, 2020

/run-unit-test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants