Skip to content

ddl: fix placement rules compatibility with tiflash#22065

Merged
ti-srebot merged 11 commits intopingcap:masterfrom
xhebox:master
Jan 12, 2021
Merged

ddl: fix placement rules compatibility with tiflash#22065
ti-srebot merged 11 commits intopingcap:masterfrom
xhebox:master

Conversation

@xhebox
Copy link
Contributor

@xhebox xhebox commented Dec 29, 2020 •
edited
Loading

What problem does this PR solve?

Issue: close #22171

Problem Summary: When PD scheduels a partition to tiflash, TiFlash will validate the keyrange if there is a _r suffix. Using GenTableRecordPrefix instead of GenTablePrefix can fix this problem.

But placement rules are not compatible with the old set tiflash replica syntax, so the second commit simply forbids using +engine=tiflash, and will add -engine=tiflash to every rule to avoid scheduels to tiflash instance. Maybe that will be reverted in the future.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
set global tidb_enable_alter_placement=on;
drop table if exists test.t1;
create table test.t1 (a int) partition by hash(a) partitions 2;
alter table test.t1 alter partition p1 alter placement policy CONSTRAINTS='["+engine=tiflash"]' ROLE=follower REPLICAS=1;
(8234, 'Invalid placement policy \'alter placement policy constraints=\'["+engine=tiflash"]\' role=follower replicas=1\': unsupported label constraint \'+engine=tiflash\'')

Release note

  • Forbid adding tiflash replica by placement rules in SQL

@xhebox xhebox marked this pull request as draft December 29, 2020 06:26
@xhebox xhebox marked this pull request as ready for review December 29, 2020 06:30
djshow832
djshow832 reviewed Dec 30, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

Add some cases to placement_sql_test.go to test if the placement rules generated by SQL are right in these scenarios:

  1. Whether the placement rules all contain "-engine=tiflash".
  2. When the user also defines "-engine=tiflash", the placement rules should only contain one "-engine=tiflash" constraint.
  3. CONSTRAINTS='{"+zone=bj":1}' REPLICAS=2, the extra count should also contain "-engine=tiflash" constraint. (Maybe?)

@xhebox
Copy link
Contributor Author

xhebox commented Dec 30, 2020 •
edited
Loading

Add some cases to placement_sql_test.go to test if the placement rules generated by SQL are right in these scenarios.

@djshow832 I can not add tests in placement_sql_test.go. Mock test wont update rule cache. Putting it into placement_rule_test.go needs too many modifications(and can not cover the test of 3). I would like to leave it to automated-test.

Or maybe manual test in patition_test.go?

djshow832
djshow832 reviewed Dec 30, 2020
View reviewed changes
djshow832
djshow832 reviewed Dec 31, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 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 Dec 31, 2020
wjhuang2016
wjhuang2016 reviewed Jan 5, 2021
View reviewed changes
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

Please create an issue for this problem.

wjhuang2016
wjhuang2016 reviewed Jan 5, 2021
View reviewed changes
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

The first commit fixed that. It is not convenient for a reviewer to find your first commit.
You can write: "Using GenTableRecordPrefix instead of GenTablePrefix can fix this problem."
Besides, is there any test for this change?

wjhuang2016
wjhuang2016 reviewed Jan 5, 2021
View reviewed changes
@xhebox
Copy link
Contributor Author

xhebox commented Jan 5, 2021 •
edited
Loading

@wjhuang2016 I will try if TestDDLCallback can test it. The problem is that rule cache will not refresh without a working PD. I can not check the result after executing placement rules SQL DDL jobs.

@wjhuang2016
Copy link
Member

wjhuang2016 commented Jan 5, 2021

@wjhuang2016 I will try if TestDDLCallback can test it. The problem is that rule cache will not refresh without a working PD.

You can do a manual test. Start a cluster to test it.

@xhebox
Copy link
Contributor Author

xhebox commented Jan 5, 2021 •
edited
Loading

You can do a manual test. Start a cluster to test it

A manual test is already done, of course.

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jan 7, 2021
@djshow832 djshow832 requested a review from wjhuang2016 January 7, 2021 06:55
xhebox added 10 commits January 12, 2021 17:31
@xhebox
ddl: fix tflash crash due to placement rule
070f7ff 
TiFlash will compare the the record prefix too.

Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
ddl: no adding tiflash replica by placement rules
0d6ee7b 
Though the previous commit fixed the compatibility between tiflash and
placement rules. But it is still not caompatible with the old `set
tiflash replica` syntax. Thus this commit banned the usage and avoid
using tiflash by appending a hidden label to filter tiflash instance.

Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
ddl: add test case
68fb4cf 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
ddl: fix fmt
3da5a61 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
ddl: address comment
f6b5dfd 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
ddl: fix test
06873ec 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
ddl: use const everywhere
69a6614 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
ddl: fix test
d90c79b 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
ddl: also add tikv label
29653a1 
Signed-off-by: xhe <xw897002528@gmail.com>
@xhebox
ddl: add comment
90d00a9 
Signed-off-by: xhe <xw897002528@gmail.com>
wjhuang2016
wjhuang2016 reviewed Jan 12, 2021
View reviewed changes
Copy link
Member

@wjhuang2016 wjhuang2016 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 Jan 12, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 12, 2021
@wjhuang2016
Copy link
Member

wjhuang2016 commented Jan 12, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 12, 2021
@ti-srebot
Copy link
Contributor

ti-srebot commented Jan 12, 2021

/run-all-tests

@ti-srebot ti-srebot merged commit ba8223f into pingcap:master Jan 12, 2021
@Yisaer Yisaer mentioned this pull request Jan 25, 2021
Merged
@xhebox xhebox mentioned this pull request Jul 7, 2021
Closed
This was referenced Dec 31, 2024
Closed
Closed
Merged
This was referenced Jan 20, 2025
Merged
Merged
Merged
Closed
Closed
@JaySon-Huang JaySon-Huang mentioned this pull request Feb 7, 2025
Merged
12 tasks
This was referenced Feb 19, 2025
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjhuang2016 wjhuang2016 wjhuang2016 left review comments

@djshow832 djshow832 djshow832 left review comments

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

@AilinKid AilinKid Awaiting requested review from AilinKid

Assignees

No one assigned

Labels

sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. 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.

ddl: placement rules in SQL is not compatible with TiFlash

4 participants

@xhebox @wjhuang2016 @ti-srebot @djshow832