From 93d28b9d3ffe2726640a72342a3c517f88bbd37d Mon Sep 17 00:00:00 2001 From: xhe Date: Fri, 24 Dec 2021 14:55:47 +0800 Subject: [PATCH] placement: give default 2 followers for non-sugar syntax (#31000) --- ddl/placement/bundle.go | 36 +++++++++++++----------------------- ddl/placement/bundle_test.go | 5 ++++- parser/parser.go | 7 ++++++- parser/parser.y | 7 ++++++- parser/parser_test.go | 6 ++++++ 5 files changed, 35 insertions(+), 26 deletions(-) diff --git a/ddl/placement/bundle.go b/ddl/placement/bundle.go index fe160c7653565..02e8bdcd5a4c9 100644 --- a/ddl/placement/bundle.go +++ b/ddl/placement/bundle.go @@ -85,33 +85,23 @@ func NewBundleFromConstraintsOptions(options *model.PlacementSettings) (*Bundle, return nil, fmt.Errorf("%w: LeaderConstraints conflicts with Constraints", err) } } - if len(LeaderConstraints) > 0 { - Rules = append(Rules, NewRule(Leader, 1, LeaderConstraints)) - } else if followerCount == 0 { - return nil, fmt.Errorf("%w: you must at least provide common/leader constraints, or set some followers", ErrInvalidPlacementOptions) - } - - if followerCount > 0 { - // if user did not specify leader, add one - if len(LeaderConstraints) == 0 { - Rules = append(Rules, NewRule(Leader, 1, NewConstraintsDirect())) - } + Rules = append(Rules, NewRule(Leader, 1, LeaderConstraints)) - FollowerRules, err := NewRules(Voter, followerCount, followerConstraints) - if err != nil { - return nil, fmt.Errorf("%w: invalid FollowerConstraints", err) - } - for _, rule := range FollowerRules { - for _, cnst := range CommonConstraints { - if err := rule.Constraints.Add(cnst); err != nil { - return nil, fmt.Errorf("%w: FollowerConstraints conflicts with Constraints", err) - } + if followerCount == 0 { + followerCount = 2 + } + FollowerRules, err := NewRules(Voter, followerCount, followerConstraints) + if err != nil { + return nil, fmt.Errorf("%w: invalid FollowerConstraints", err) + } + for _, rule := range FollowerRules { + for _, cnst := range CommonConstraints { + if err := rule.Constraints.Add(cnst); err != nil { + return nil, fmt.Errorf("%w: FollowerConstraints conflicts with Constraints", err) } } - Rules = append(Rules, FollowerRules...) - } else if followerConstraints != "" { - return nil, fmt.Errorf("%w: specify follower constraints without specify how many followers to be placed", ErrInvalidPlacementOptions) } + Rules = append(Rules, FollowerRules...) if learnerCount > 0 { LearnerRules, err := NewRules(Learner, learnerCount, learnerConstraints) diff --git a/ddl/placement/bundle_test.go b/ddl/placement/bundle_test.go index 3c11559f06304..c2a5b8c05dfad 100644 --- a/ddl/placement/bundle_test.go +++ b/ddl/placement/bundle_test.go @@ -572,7 +572,10 @@ func (s *testBundleSuite) TestNewBundleFromOptions(c *C) { LeaderConstraints: "[+region=as]", FollowerConstraints: "[-region=us]", }, - err: ErrInvalidPlacementOptions, + output: []*Rule{ + NewRule(Leader, 1, NewConstraintsDirect(NewConstraintDirect("region", In, "as"))), + NewRule(Voter, 2, NewConstraintsDirect(NewConstraintDirect("region", NotIn, "us"))), + }, }) tests = append(tests, TestCase{ diff --git a/parser/parser.go b/parser/parser.go index a4fa6c451eed0..c93d5f37e6ac7 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -11289,7 +11289,12 @@ yynewstate: } case 10: { - parser.yyVAL.item = &ast.PlacementOption{Tp: ast.PlacementOptionFollowerCount, UintValue: yyS[yypt-0].item.(uint64)} + cnt := yyS[yypt-0].item.(uint64) + if cnt == 0 { + yylex.AppendError(yylex.Errorf("FOLLOWERS must be positive")) + return 1 + } + parser.yyVAL.item = &ast.PlacementOption{Tp: ast.PlacementOptionFollowerCount, UintValue: cnt} } case 11: { diff --git a/parser/parser.y b/parser/parser.y index 91e3b05918fc9..b79e7bccd2591 100644 --- a/parser/parser.y +++ b/parser/parser.y @@ -1526,7 +1526,12 @@ DirectPlacementOption: } | "FOLLOWERS" EqOpt LengthNum { - $$ = &ast.PlacementOption{Tp: ast.PlacementOptionFollowerCount, UintValue: $3.(uint64)} + cnt := $3.(uint64) + if cnt == 0 { + yylex.AppendError(yylex.Errorf("FOLLOWERS must be positive")) + return 1 + } + $$ = &ast.PlacementOption{Tp: ast.PlacementOptionFollowerCount, UintValue: cnt} } | "VOTERS" EqOpt LengthNum { diff --git a/parser/parser_test.go b/parser/parser_test.go index 1c5ed6b4e9987..a9587bdba5724 100644 --- a/parser/parser_test.go +++ b/parser/parser_test.go @@ -2338,6 +2338,7 @@ func TestDDL(t *testing.T) { {`create table t (c int) regions="us,3";`, true, "CREATE TABLE `t` (`c` INT) REGIONS = 'us,3'"}, {`create table t (c int) followers="us,3";`, false, ""}, {`create table t (c int) followers=3;`, true, "CREATE TABLE `t` (`c` INT) FOLLOWERS = 3"}, + {`create table t (c int) followers=0;`, false, ""}, {`create table t (c int) voters="us,3";`, false, ""}, {`create table t (c int) voters=3;`, true, "CREATE TABLE `t` (`c` INT) VOTERS = 3"}, {`create table t (c int) learners="us,3";`, false, ""}, @@ -2353,6 +2354,7 @@ func TestDDL(t *testing.T) { {`create table t (c int) /*T![placement] regions="us,3" */;`, true, "CREATE TABLE `t` (`c` INT) REGIONS = 'us,3'"}, {`create table t (c int) /*T![placement] followers="us,3 */";`, false, ""}, {`create table t (c int) /*T![placement] followers=3 */;`, true, "CREATE TABLE `t` (`c` INT) FOLLOWERS = 3"}, + {`create table t (c int) /*T![placement] followers=0 */;`, false, ""}, {`create table t (c int) /*T![placement] voters="us,3" */;`, false, ""}, {`create table t (c int) /*T![placement] primary_region="us" regions="us,3" */;`, true, "CREATE TABLE `t` (`c` INT) PRIMARY_REGION = 'us' REGIONS = 'us,3'"}, {"create table t (c int) /*T![placement] placement policy=`x` */;", true, "CREATE TABLE `t` (`c` INT) PLACEMENT POLICY = `x`"}, @@ -2361,6 +2363,7 @@ func TestDDL(t *testing.T) { {`alter table t primary_region="us";`, true, "ALTER TABLE `t` PRIMARY_REGION = 'us'"}, {`alter table t regions="us,3";`, true, "ALTER TABLE `t` REGIONS = 'us,3'"}, {`alter table t followers=3;`, true, "ALTER TABLE `t` FOLLOWERS = 3"}, + {`alter table t followers=0;`, false, ""}, {`alter table t voters=3;`, true, "ALTER TABLE `t` VOTERS = 3"}, {`alter table t learners=3;`, true, "ALTER TABLE `t` LEARNERS = 3"}, {`alter table t schedule="even";`, true, "ALTER TABLE `t` SCHEDULE = 'even'"}, @@ -2375,6 +2378,7 @@ func TestDDL(t *testing.T) { {`create database t primary_region="us";`, true, "CREATE DATABASE `t` PRIMARY_REGION = 'us'"}, {`create database t regions="us,3";`, true, "CREATE DATABASE `t` REGIONS = 'us,3'"}, {`create database t followers=3;`, true, "CREATE DATABASE `t` FOLLOWERS = 3"}, + {`create database t followers=0;`, false, ""}, {`create database t voters=3;`, true, "CREATE DATABASE `t` VOTERS = 3"}, {`create database t learners=3;`, true, "CREATE DATABASE `t` LEARNERS = 3"}, {`create database t schedule="even";`, true, "CREATE DATABASE `t` SCHEDULE = 'even'"}, @@ -2390,6 +2394,7 @@ func TestDDL(t *testing.T) { {`alter database t primary_region="us";`, true, "ALTER DATABASE `t` PRIMARY_REGION = 'us'"}, {`alter database t regions="us,3";`, true, "ALTER DATABASE `t` REGIONS = 'us,3'"}, {`alter database t followers=3;`, true, "ALTER DATABASE `t` FOLLOWERS = 3"}, + {`alter database t followers=0;`, false, ""}, {`alter database t voters=3;`, true, "ALTER DATABASE `t` VOTERS = 3"}, {`alter database t learners=3;`, true, "ALTER DATABASE `t` LEARNERS = 3"}, {`alter database t schedule="even";`, true, "ALTER DATABASE `t` SCHEDULE = 'even'"}, @@ -3382,6 +3387,7 @@ func TestDDL(t *testing.T) { {"create placement policy x primary_region='us'", true, "CREATE PLACEMENT POLICY `x` PRIMARY_REGION = 'us'"}, {"create placement policy x region='us, 3'", false, ""}, {"create placement policy x followers=3", true, "CREATE PLACEMENT POLICY `x` FOLLOWERS = 3"}, + {"create placement policy x followers=0", false, ""}, {"create placement policy x voters=3", true, "CREATE PLACEMENT POLICY `x` VOTERS = 3"}, {"create placement policy x learners=3", true, "CREATE PLACEMENT POLICY `x` LEARNERS = 3"}, {"create placement policy x schedule='even'", true, "CREATE PLACEMENT POLICY `x` SCHEDULE = 'even'"},