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: check the tiflash replica count when setting tiflash replica #18826

Merged
merged 19 commits into from
Aug 3, 2020
Merged

ddl: check the tiflash replica count when setting tiflash replica #18826

merged 19 commits into from
Aug 3, 2020

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Jul 28, 2020 •
edited
Loading

What problem does this PR solve?

Fix #18810

When setting the tiflash replica count, TiDB should check the replica count not more than the count of tiflash store, otherwise, the tiflash region replica will never available.

example:

test> alter table t set tiflash replica 2 location labels 'a','b';
(1105, 'the tiflash replica count: 2 should less than the total tiflash server count: 1')
test> alter table t set tiflash replica 1 location labels 'a','b';
Query OK, 0 rows affected

What is changed and how it works?

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note

  • Check the tiflash replica count when setting tiflash replica

@crazycs520
init
a3794f2 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
fix test
aba5f34 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
add test
0c9b6c8 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520 crazycs520 requested a review from a team as a code owner July 28, 2020 08:36
@crazycs520 crazycs520 requested review from XuHuaiyu and removed request for a team July 28, 2020 08:36
@crazycs520 crazycs520 added the sig/sql-infra SIG: SQL Infra label Jul 28, 2020
@crazycs520
make ci happy
33ec64d 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@github-actions github-actions bot added the sig/execution SIG execution label Jul 28, 2020
@crazycs520
add check in owner
6ab9e40 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@codecov
Copy link

codecov bot commented Jul 28, 2020 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #18826   +/-   ##
===========================================
  Coverage   79.0778%   79.0778%           
===========================================
  Files           549        549           
  Lines        148742     148742           
===========================================
  Hits         117622     117622           
  Misses        21635      21635           
  Partials       9485       9485

@crazycs520
fix race test
119e054 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
Merge branch 'master' into add-check-replica
b50e92b
@crazycs520
Copy link
Contributor Author

/rebuild

AilinKid
AilinKid reviewed Jul 30, 2020
View reviewed changes
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

ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
infoschema/tables.go Outdated Show resolved Hide resolved
AilinKid and others added 4 commits July 30, 2020 11:08
@AilinKid
Merge branch 'master' into add-check-replica
dd73e98
@crazycs520 @AilinKid
Update ddl/ddl_api.go
959f40d 
Co-authored-by: Arenatlx <ailinsilence4@gmail.com>
@crazycs520
address comment
7d3ad46 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
Merge branch 'master' into add-check-replica
5eb5cee
@JaySon-Huang

This comment has been minimized.

Sign in to view
@crazycs520
Copy link
Contributor Author

@JaySon-Huang, the tombstone tiflash store is already been remove before the check.

@crazycs520
Copy link
Contributor Author

/run-all-tests

AilinKid
AilinKid reviewed Aug 3, 2020
View reviewed changes
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

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 3, 2020
@AilinKid
Copy link
Contributor

AilinKid commented Aug 3, 2020

/run-check_dev_2

@crazycs520
Copy link
Contributor Author

/run-unit-test

tangenta
tangenta reviewed Aug 3, 2020
View reviewed changes
Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 3, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 3, 2020
@crazycs520 crazycs520 added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Aug 3, 2020
@crazycs520 crazycs520 merged commit 6088e58 into pingcap:master Aug 3, 2020
@crazycs520
Copy link
Contributor Author

/run-cherry-picker

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 3, 2020
@crazycs520 @ti-srebot
cherry pick pingcap#18826 to release-4.0
6f72052 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 3, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #18943

ti-srebot added a commit that referenced this pull request Aug 3, 2020
@ti-srebot
ddl: check the tiflash replica count when setting tiflash replica (#1…
dd67170 
…8826) (#18943)

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tangenta tangenta tangenta left review comments

@AilinKid AilinKid AilinKid left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

Assignees
No one assigned
Labels
priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Explicitly report error for TiFlash replica count > TiFlash node count
5 participants
@crazycs520 @JaySon-Huang @AilinKid @ti-srebot @tangenta