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

ddl: defining the placement rule of a leader requires REPLICAS option #20813

Merged
merged 10 commits into from
Nov 5, 2020
Merged

ddl: defining the placement rule of a leader requires REPLICAS option #20813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0929a61
ddl: defining the placement rule of a leader requires `REPLICAS` option
TszKitLo40 Nov 3, 2020
cc9448d
fix unit test.
TszKitLo40 Nov 4, 2020
383f42a
nitpick.
TszKitLo40 Nov 4, 2020
7751a09
Update ddl/placement_sql_test.go
TszKitLo40 Nov 4, 2020
5277318
Merge branch 'master' into fix20751
AilinKid Nov 4, 2020
de17f58
Merge branch 'master' into fix20751
AilinKid Nov 4, 2020
f38dd4c
Merge branch 'master' into fix20751
ti-srebot Nov 4, 2020
7d0899e
Add constrait of replicas when the role is leader.
TszKitLo40 Nov 4, 2020
a3a8837
Merge branch 'fix20751' of https://github.com/TszKitLo40/tidb into fi…
TszKitLo40 Nov 4, 2020
7bb93cc
Merge branch 'master' into fix20751
ti-srebot Nov 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ddl/ddl_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5846,6 +5846,12 @@ func buildPlacementSpecs(bundle *placement.Bundle, specs []*ast.PlacementSpec) (
case ast.PlacementRoleFollower:
role = placement.Follower
case ast.PlacementRoleLeader:
if spec.Replicas == 0 {
spec.Replicas = 1
}
Comment on lines +5849 to +5851
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the user sets it to 0 explicitly?
e.g. leader=role replicas=0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case has been treated in the parser.

Copy link
Contributor

Choose a reason for hiding this comment

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

why not just throw "Invalid placement option REPLICAS, it is should be identified"? @djshow832

Copy link
Contributor

Choose a reason for hiding this comment

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

The number of leaders can be only 1. It's unnecessary to declare it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

if spec.Replicas > 1 {
err = errors.Errorf("replicas can only be 1 when the role is leader")
}
role = placement.Leader
case ast.PlacementRoleLearner:
role = placement.Learner
Expand Down
71 changes: 40 additions & 31 deletions ddl/placement_sql_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,42 +41,42 @@ PARTITION BY RANGE (c) (
_, err := tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+zone=sh"]'
role=leader
role=follower
replicas=3`)
c.Assert(err, IsNil)

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+ zone = sh ", "- zone = bj "]'
role=leader
role=follower
replicas=3`)
c.Assert(err, IsNil)

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='{"+ zone = sh ": 1}'
role=leader
role=follower
replicas=3`)
c.Assert(err, IsNil)

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='{"+ zone = sh, -zone = bj ": 1}'
role=leader
role=follower
replicas=3`)
c.Assert(err, IsNil)

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='{"+ zone = sh ": 1, "- zone = bj": 2}'
role=leader
role=follower
replicas=3`)
c.Assert(err, IsNil)

_, err = tk.Exec(`alter table t1 alter partition p0
alter placement policy
constraints='{"+ zone = sh, -zone = bj ": 1}'
role=leader
role=follower
replicas=3`)
c.Assert(err, IsNil)

Expand All @@ -89,52 +89,52 @@ drop placement policy
_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+ zone = sh "]'
role=leader
role=follower
replicas=3,
add placement policy
constraints='{"+ zone = sh, -zone = bj ": 1}'
role=leader
role=follower
replicas=3`)
c.Assert(err, IsNil)

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+ zone = sh "]'
role=leader
role=follower
replicas=3,
add placement policy
constraints='{"+zone=sh,+zone=bj":1,"+zone=sh,+zone=bj":1}'
role=leader
role=follower
replicas=3`)
c.Assert(err, IsNil)

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='{"+ zone = sh ": 1, "- zone = bj,+zone=sh": 2}'
role=leader
role=follower
replicas=3,
alter placement policy
constraints='{"+ zone = sh, -zone = bj ": 1}'
role=leader
role=follower
replicas=3`)
c.Assert(err, IsNil)

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+zone=sh", "-zone=bj"]'
role=leader
role=follower
replicas=3,
add placement policy
constraints='{"+zone=sh": 1}'
role=leader
role=follower
replicas=3,
add placement policy
constraints='{"+zone=sh,+zone=bj":1,"+zone=sh,+zone=bj":1}'
role=leader
role=follower
replicas=3,
alter placement policy
constraints='{"+zone=sh": 1, "-zon =bj,+zone=sh": 1}'
role=leader
role=follower
replicas=3`)
c.Assert(err, IsNil)

Expand All @@ -158,103 +158,103 @@ drop placement policy
_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints=',,,'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*array or object.*")

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='[,,,'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*invalid character.*")

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='{,,,'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*invalid character.*")

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='{"+ zone = sh ": 1, "- zone = bj": 2}'
role=leader
role=follower
replicas=2`)
c.Assert(err, ErrorMatches, ".*should be larger or equal to the number of total replicas.*")

// checkPlacementSpecConstraint
_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='[",,,"]'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*label constraint should be in format.*")

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+ "]'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*label constraint should be in format.*")

// unknown operation
_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["0000"]'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*label constraint should be in format.*")

// without =
_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+000"]'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*label constraint should be in format.*")

// empty key
_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+ =zone1"]'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*label constraint should be in format.*")

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+ = z"]'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*label constraint should be in format.*")

// empty value
_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+zone="]'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*label constraint should be in format.*")

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='["+z = "]'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*label constraint should be in format.*")

_, err = tk.Exec(`alter table t1 alter partition p
add placement policy
constraints='["+zone=sh"]'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*Unknown partition.*")

_, err = tk.Exec(`alter table t1 alter partition p0
add placement policy
constraints='{"+ zone = sh, -zone = bj ": -1}'
role=leader
role=follower
replicas=3`)
c.Assert(err, ErrorMatches, ".*count should be positive.*")

Expand All @@ -271,9 +271,18 @@ add placement policy
_, err = tk.Exec(`alter table t1 alter partition p
add placement policy
constraints='["+zone=sh"]'
role=leader
role=follower
replicas=3`)
c.Assert(ddl.ErrPartitionMgmtOnNonpartitioned.Equal(err), IsTrue)

// issue 20751
tk.MustExec("drop table if exists t_part_pk_id")
tk.MustExec("create TABLE t_part_pk_id (c1 int,c2 int) partition by range(c1 div c2 ) (partition p0 values less than (2))")
_, err = tk.Exec(`alter table t_part_pk_id alter partition p0 add placement policy constraints='["+host=store1"]' role=leader;`)
c.Assert(err, IsNil)
_, err = tk.Exec(`alter table t_part_pk_id alter partition p0 add placement policy constraints='["+host=store1"]' role=leader replicas=3;`)
c.Assert(err, ErrorMatches, ".*replicas can only be 1 when the role is leader")
tk.MustExec("drop table t_part_pk_id")
}

func (s *testDBSuite1) TestPlacementPolicyCache(c *C) {
Expand Down