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

domain: avoid check schema changed when change is tiflash replica status #13034

Merged
merged 10 commits into from
Nov 5, 2019

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Oct 30, 2019

What problem does this PR solve?

Some DDL change no need to add to schemaChecker.
Such as:

  • ActionUpdateTiFlashReplicaStatus
  • ActionSetTiFlashReplica

Maybe below DDL type also no need to add to schemaChecker:

  • ActionCreateSchema
  • ActionCreateTable
  • ActionShardRowID
  • ActionModifyTableComment
  • ActionCreateView
  • ActionRecoverTable
  • ActionRepairTable

What is changed and how it works?

Check List

Tests

Code changes

  • Has exported function/method change

Side effects

  • Possible performance regression

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Write release note for bug-fix or new feature.

@codecov
Copy link

codecov bot commented Oct 30, 2019

Codecov Report

Merging #13034 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13034   +/-   ##
===========================================
  Coverage   80.1514%   80.1514%           
===========================================
  Files           468        468           
  Lines        111177     111177           
===========================================
  Hits          89110      89110           
  Misses        15241      15241           
  Partials       6826       6826

domain/domain.go Outdated
@@ -254,6 +254,9 @@ func (do *Domain) tryLoadSchemaDiffs(m *meta.Meta, usedVersion, newVersion int64
if err != nil {
return false, nil, err
}
if diff.Type == model.ActionUpdateTiFlashReplicaStatus || diff.Type == model.ActionSetTiFlashReplica {
Copy link
Member

Choose a reason for hiding this comment

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

Since we may add more skippable types, how about adding a function like skippableSchemaCheckTypes(diff.Type)?

@Deardrops
Copy link
Contributor

/run-all-tests

@zyxbest
Copy link
Contributor

zyxbest commented Nov 1, 2019

/run-mybatis-test

@crazycs520
Copy link
Contributor Author

/run-all-tests

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 LGTM

tblIDs = append(tblIDs, ids...)
}
builder.Build()
return true, tblIDs, nil
}

func canSkipSchemaCheckerDDL(tp model.ActionType) bool {
switch tp {
case model.ActionUpdateTiFlashReplicaStatus, model.ActionSetTiFlashReplica:
Copy link
Contributor

@AilinKid AilinKid Nov 5, 2019

Choose a reason for hiding this comment

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

By now two options here, how about use if A||B? maybe fast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

em... I prefer switch😂

@AilinKid
Copy link
Contributor

AilinKid commented Nov 5, 2019

/run-unit-test

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 5, 2019
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 crazycs520 added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 5, 2019
@crazycs520 crazycs520 removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 5, 2019
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

@AilinKid AilinKid added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 5, 2019
@crazycs520
Copy link
Contributor Author

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 5, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 5, 2019

Your auto merge job has been accepted, waiting for 13130, 12895, 13108, 13107, 12628, 13089, 13090

@sre-bot
Copy link
Contributor

sre-bot commented Nov 5, 2019

/run-all-tests

@sre-bot sre-bot merged commit 354d872 into pingcap:master Nov 5, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
crazycs520 added a commit to crazycs520/tidb that referenced this pull request Feb 21, 2020
…tus (pingcap#13034)

Signed-off-by: crazycs <crazycs520@gmail.com>
bb7133 pushed a commit that referenced this pull request Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM. type/usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants