From a0160612a8fcda93097ff9ca42165139cab34fca Mon Sep 17 00:00:00 2001 From: xhe Date: Tue, 11 Aug 2020 15:05:05 +0800 Subject: [PATCH] ddl: support `ALTER PLACEMENT` clause (#19065) * ddl: check count here * ddl: drop all rules that will be overrided * ddl: change ID format * ddl: local placement deifinition * ddl: support `ALTER PLACEMENT` clause * ddl: add test cases * ddl: fix ID format * ddl: copy comments too * ddl: allow ignore replicas with dict constraints * ddl: rename COUNT to REPLICAS * ddl: clone method for RuleOp * ddl: checkPlacementSpec => buildPlacementSpec * ddl: remove TODO It is possible to incrementally add replicas. Consider drop later. Let us support it correctly first. * ddl: ID format Datbase/schema ID is added back. A common prefix will improve the efficience of PD batch operation. Drop a whole db/table will only need one loop for rules. * ddl: move const/util to placement package * ddl: fix test * ddl: fix format error * ddl: error check fix * ddl: use an explicit condition flag * ddl: adapt parser changes * ddl: fix go.mod/simplify code path * ddl: go mod tidy * ddl: improve tests * ddl: return new rules instead of appending * ddl: return constraints instead of appending * ddl: fix test * ddl: one more test * ddl: remove rules rollback, meaningless * ddl: fix slice * ddl: add period to comments * Update ddl/ddl_api.go Co-authored-by: tangenta * ddl: remove unused arguments * infosync: do not request PD if no ruleOperations * ddl: a new test suite Co-authored-by: tangenta Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com> --- ddl/ddl_api.go | 171 +++++++++--------- ddl/partition.go | 10 +- ddl/placement/const.go | 31 ++++ ddl/placement/types.go | 99 +++++++++++ ddl/placement/utils.go | 71 ++++++++ ddl/placement_rule_test.go | 353 +++++++++++++++++++++---------------- ddl/placement_sql_test.go | 252 ++++++++++++++++++++++++++ domain/infosync/info.go | 10 +- go.mod | 2 +- go.sum | 2 + util/testleak/leaktest.go | 2 - 11 files changed, 752 insertions(+), 251 deletions(-) create mode 100644 ddl/placement/const.go create mode 100644 ddl/placement/types.go create mode 100644 ddl/placement/utils.go create mode 100644 ddl/placement_sql_test.go diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 9febbd701c..f3ce2213bb 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -36,8 +36,8 @@ import ( "github.com/pingcap/parser/model" "github.com/pingcap/parser/mysql" field_types "github.com/pingcap/parser/types" - "github.com/pingcap/pd/v4/server/schedule/placement" "github.com/pingcap/tidb/config" + "github.com/pingcap/tidb/ddl/placement" "github.com/pingcap/tidb/expression" "github.com/pingcap/tidb/infoschema" "github.com/pingcap/tidb/kv" @@ -62,16 +62,6 @@ import ( const expressionIndexPrefix = "_V$" -const placementRuleDefaultGroupID = "TiDB_DDL" - -const ( - placementRuleIndexDefault int = iota - placementRuleIndexDatabase - placementRuleIndexTable - placementRuleIndexPartition - placementRuleIndexIndex -) - func (d *ddl) CreateSchema(ctx sessionctx.Context, schema model.CIStr, charsetInfo *ast.CharsetOpt) error { dbInfo := &model.DBInfo{Name: schema} if charsetInfo != nil { @@ -5165,59 +5155,17 @@ func (d *ddl) AlterIndexVisibility(ctx sessionctx.Context, ident ast.Ident, inde return errors.Trace(err) } -func checkPlacementLabelConstraint(label string) (placement.LabelConstraint, error) { - r := placement.LabelConstraint{} - - if len(label) < 4 { - return r, errors.Errorf("label constraint should be in format '{+|-}key=value', but got '%s'", label) - } - - var op placement.LabelConstraintOp - switch label[0] { - case '+': - op = placement.In - case '-': - op = placement.NotIn - default: - return r, errors.Errorf("label constraint should be in format '{+|-}key=value', but got '%s'", label) - } - - kv := strings.Split(label[1:], "=") - if len(kv) != 2 { - return r, errors.Errorf("label constraint should be in format '{+|-}key=value', but got '%s'", label) - } - - key := strings.TrimSpace(kv[0]) - if key == "" { - return r, errors.Errorf("label constraint should be in format '{+|-}key=value', but got '%s'", label) - } - - val := strings.TrimSpace(kv[1]) - if val == "" { - return r, errors.Errorf("label constraint should be in format '{+|-}key=value', but got '%s'", label) - } - - r.Key = key - r.Op = op - r.Values = []string{val} - return r, nil -} - -func checkPlacementLabelConstraints(rule *placement.Rule, labels []string) error { - for _, str := range labels { - label, err := checkPlacementLabelConstraint(strings.TrimSpace(str)) - if err != nil { - return err - } - rule.LabelConstraints = append(rule.LabelConstraints, label) - } - return nil -} - -func checkPlacementSpecConstraint(rules []*placement.Rule, rule *placement.Rule, cnstr string) ([]*placement.Rule, error) { - cnstr = strings.TrimSpace(cnstr) +func buildPlacementSpecReplicasAndConstraint(rule *placement.RuleOp, replicas uint64, cnstr string) ([]*placement.RuleOp, error) { var err error + cnstr = strings.TrimSpace(cnstr) + rules := make([]*placement.RuleOp, 0, 1) if len(cnstr) > 0 && cnstr[0] == '[' { + // can not emit REPLICAS with an array label + if replicas == 0 { + return rules, errors.Errorf("array CONSTRAINTS should be with a positive REPLICAS") + } + rule.Count = int(replicas) + constraints := []string{} err = json.Unmarshal([]byte(cnstr), &constraints) @@ -5225,7 +5173,7 @@ func checkPlacementSpecConstraint(rules []*placement.Rule, rule *placement.Rule, return rules, err } - err = checkPlacementLabelConstraints(rule, constraints) + rule.LabelConstraints, err = placement.CheckLabelConstraints(constraints) if err != nil { return rules, err } @@ -5238,30 +5186,42 @@ func checkPlacementSpecConstraint(rules []*placement.Rule, rule *placement.Rule, return rules, err } + ruleCnt := int(replicas) for labels, cnt := range constraints { - newRule := &placement.Rule{} - *newRule = *rule + newRule := rule.Clone() if cnt <= 0 { - err = errors.Errorf("count should be non-positive, but got %d", cnt) + err = errors.Errorf("count should be positive, but got %d", cnt) break } - // TODO: handle or remove rule.Count in later commits - rule.Count -= cnt + + if replicas != 0 { + ruleCnt -= cnt + if ruleCnt < 0 { + err = errors.Errorf("REPLICAS should be larger or equal to the number of total replicas, but got %d", replicas) + break + } + } newRule.Count = cnt - err = checkPlacementLabelConstraints(newRule, strings.Split(strings.TrimSpace(labels), ",")) + + newRule.LabelConstraints, err = placement.CheckLabelConstraints(strings.Split(strings.TrimSpace(labels), ",")) if err != nil { break } rules = append(rules, newRule) } + rule.Count = ruleCnt + + if rule.Count > 0 { + rules = append(rules, rule) + } } else { err = errors.Errorf("constraint should be a JSON array or object, but got '%s'", cnstr) } return rules, err } -func checkPlacementSpecs(specs []*ast.PlacementSpec) ([]*placement.Rule, error) { - rules := make([]*placement.Rule, 0, len(specs)) +func buildPlacementSpecs(specs []*ast.PlacementSpec) ([]*placement.RuleOp, error) { + rules := make([]*placement.RuleOp, 0, len(specs)) var err error var sb strings.Builder @@ -5269,35 +5229,64 @@ func checkPlacementSpecs(specs []*ast.PlacementSpec) ([]*placement.Rule, error) restoreCtx := format.NewRestoreCtx(restoreFlags, &sb) for _, spec := range specs { - rule := &placement.Rule{ - GroupID: placementRuleDefaultGroupID, - Count: int(spec.Replicas), - Override: true, + rule := &placement.RuleOp{ + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + Override: true, + }, } - switch spec.Tp { - case ast.PlacementAdd: + switch spec.Role { + case ast.PlacementRoleFollower: + rule.Role = placement.Follower + case ast.PlacementRoleLeader: + rule.Role = placement.Leader + case ast.PlacementRoleLearner: + rule.Role = placement.Learner + case ast.PlacementRoleVoter: + rule.Role = placement.Voter default: - err = errors.Errorf("unknown action type: %d", spec.Tp) + err = errors.Errorf("unknown role: %d", spec.Role) } if err == nil { - switch spec.Role { - case ast.PlacementRoleFollower: - rule.Role = placement.Follower - case ast.PlacementRoleLeader: - rule.Role = placement.Leader - case ast.PlacementRoleLearner: - rule.Role = placement.Learner - case ast.PlacementRoleVoter: - rule.Role = placement.Voter + switch spec.Tp { + case ast.PlacementAdd: + rule.Action = placement.RuleOpAdd + case ast.PlacementAlter: + rule.Action = placement.RuleOpAdd + + // alter will overwrite all things + // drop all rules that will be overridden + newRules := rules[:0] + + for _, r := range rules { + if r.Role != rule.Role { + newRules = append(newRules, r) + } + } + + rules = newRules + + // delete previous definitions + rules = append(rules, &placement.RuleOp{ + Action: placement.RuleOpDel, + DeleteByIDPrefix: true, + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + // ROLE is useless for PD, prevent two alter statements from overriding each other + Role: rule.Role, + }, + }) default: - err = errors.Errorf("unknown role: %d", spec.Role) + err = errors.Errorf("unknown action type: %d", spec.Tp) } } if err == nil { - rules, err = checkPlacementSpecConstraint(rules, rule, spec.Constraints) + var newRules []*placement.RuleOp + newRules, err = buildPlacementSpecReplicasAndConstraint(rule, spec.Replicas, spec.Constraints) + rules = append(rules, newRules...) } if err != nil { @@ -5327,7 +5316,7 @@ func (d *ddl) AlterTablePartition(ctx sessionctx.Context, ident ast.Ident, spec return errors.Trace(err) } - rules, err := checkPlacementSpecs(spec.PlacementSpecs) + rules, err := buildPlacementSpecs(spec.PlacementSpecs) if err != nil { return errors.Trace(err) } @@ -5335,7 +5324,7 @@ func (d *ddl) AlterTablePartition(ctx sessionctx.Context, ident ast.Ident, spec startKey := hex.EncodeToString(codec.EncodeBytes(nil, tablecodec.GenTablePrefix(partitionID))) endKey := hex.EncodeToString(codec.EncodeBytes(nil, tablecodec.GenTablePrefix(partitionID+1))) for _, rule := range rules { - rule.Index = placementRuleIndexPartition + rule.Index = placement.RuleIndexPartition rule.StartKeyHex = startKey rule.EndKeyHex = endKey } diff --git a/ddl/partition.go b/ddl/partition.go index 5b3f7da7fa..3f9ff567ab 100644 --- a/ddl/partition.go +++ b/ddl/partition.go @@ -31,7 +31,7 @@ import ( "github.com/pingcap/parser/model" "github.com/pingcap/parser/mysql" "github.com/pingcap/parser/opcode" - "github.com/pingcap/pd/v4/server/schedule/placement" + "github.com/pingcap/tidb/ddl/placement" "github.com/pingcap/tidb/ddl/util" "github.com/pingcap/tidb/domain/infosync" "github.com/pingcap/tidb/expression" @@ -1451,7 +1451,7 @@ func truncateTableByReassignPartitionIDs(t *meta.Meta, tblInfo *model.TableInfo) func onAlterTablePartition(t *meta.Meta, job *model.Job) (int64, error) { var partitionID int64 - var rules []*placement.Rule + var rules []*placement.RuleOp err := job.DecodeArgs(&partitionID, &rules) if err != nil { job.State = model.JobStateCancelled @@ -1470,7 +1470,11 @@ func onAlterTablePartition(t *meta.Meta, job *model.Job) (int64, error) { } for i, rule := range rules { - rule.ID = fmt.Sprintf("%d_%d_%d_%d_%d", job.SchemaID, tblInfo.ID, partitionID, job.ID, i) + if rule.Action == placement.RuleOpDel { + rule.ID = fmt.Sprintf("%d_t%d_p%d_%s", job.SchemaID, tblInfo.ID, partitionID, rule.Role) + } else { + rule.ID = fmt.Sprintf("%d_t%d_p%d_%s_%d_%d", job.SchemaID, tblInfo.ID, partitionID, rule.Role, job.ID, i) + } } ver, err := t.GetSchemaVersion() diff --git a/ddl/placement/const.go b/ddl/placement/const.go new file mode 100644 index 0000000000..3e4df60882 --- /dev/null +++ b/ddl/placement/const.go @@ -0,0 +1,31 @@ +// Copyright 2020 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// See the License for the specific language governing permissions and +// limitations under the License. + +package placement + +// RuleDefaultGroupID is the default GroupID for all placement rules, to +// indicate that it is from TiDB_DDL statements. +const RuleDefaultGroupID = "TiDB_DDL" + +const ( + // RuleIndexDefault is the default index for a rule, check Rule.Index. + RuleIndexDefault int = iota + // RuleIndexDatabase is the index for a rule of database. + RuleIndexDatabase + // RuleIndexTable is the index for a rule of table. + RuleIndexTable + // RuleIndexPartition is the index for a rule of partition. + RuleIndexPartition + // RuleIndexIndex is the index for a rule of index. + RuleIndexIndex +) diff --git a/ddl/placement/types.go b/ddl/placement/types.go new file mode 100644 index 0000000000..b76957197b --- /dev/null +++ b/ddl/placement/types.go @@ -0,0 +1,99 @@ +// Copyright 2020 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// See the License for the specific language governing permissions and +// limitations under the License. + +package placement + +// Refer to https://github.com/pingcap/pd/issues/2701 . +// IMO, it is indeed not bad to have a copy of definition. +// After all, placement rules are communicated using an HTTP API. Loose +// coupling is a good feature. + +// PeerRoleType is the expected peer type of the placement rule. +type PeerRoleType string + +const ( + // Voter can either match a leader peer or follower peer. + Voter PeerRoleType = "voter" + // Leader matches a leader. + Leader PeerRoleType = "leader" + // Follower matches a follower. + Follower PeerRoleType = "follower" + // Learner matches a learner. + Learner PeerRoleType = "learner" +) + +// LabelConstraintOp defines how a LabelConstraint matches a store. +type LabelConstraintOp string + +const ( + // In restricts the store label value should in the value list. + // If label does not exist, `in` is always false. + In LabelConstraintOp = "in" + // NotIn restricts the store label value should not in the value list. + // If label does not exist, `notIn` is always true. + NotIn LabelConstraintOp = "notIn" + // Exists restricts the store should have the label. + Exists LabelConstraintOp = "exists" + // NotExists restricts the store should not have the label. + NotExists LabelConstraintOp = "notExists" +) + +// LabelConstraint is used to filter store when trying to place peer of a region. +type LabelConstraint struct { + Key string `json:"key,omitempty"` + Op LabelConstraintOp `json:"op,omitempty"` + Values []string `json:"values,omitempty"` +} + +// Rule is the placement rule. Check https://github.com/pingcap/pd/blob/master/server/schedule/placement/rule.go. +type Rule struct { + GroupID string `json:"group_id"` + ID string `json:"id"` + Index int `json:"index,omitempty"` + Override bool `json:"override,omitempty"` + StartKey []byte `json:"-"` + StartKeyHex string `json:"start_key"` + EndKey []byte `json:"-"` + EndKeyHex string `json:"end_key"` + Role PeerRoleType `json:"role"` + Count int `json:"count"` + LabelConstraints []LabelConstraint `json:"label_constraints,omitempty"` + LocationLabels []string `json:"location_labels,omitempty"` + IsolationLevel string `json:"isolation_level,omitempty"` +} + +// RuleOpType indicates the operation type. +type RuleOpType string + +const ( + // RuleOpAdd a placement rule, only need to specify the field *Rule. + RuleOpAdd RuleOpType = "add" + // RuleOpDel a placement rule, only need to specify the field `GroupID`, `ID`, `MatchID`. + RuleOpDel RuleOpType = "del" +) + +// RuleOp is for batching placement rule actions. +type RuleOp struct { + *Rule + Action RuleOpType `json:"action"` + DeleteByIDPrefix bool `json:"delete_by_id_prefix"` +} + +// Clone is used to clone a RuleOp that is safe to modify, without affecting the old RuleOp. +func (op *RuleOp) Clone() *RuleOp { + newOp := &RuleOp{} + *newOp = *op + newOp.Rule = &Rule{} + *newOp.Rule = *op.Rule + return newOp +} diff --git a/ddl/placement/utils.go b/ddl/placement/utils.go new file mode 100644 index 0000000000..b251b9f016 --- /dev/null +++ b/ddl/placement/utils.go @@ -0,0 +1,71 @@ +// Copyright 2020 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// See the License for the specific language governing permissions and +// limitations under the License. + +package placement + +import ( + "strings" + + "github.com/pingcap/errors" +) + +func checkLabelConstraint(label string) (LabelConstraint, error) { + r := LabelConstraint{} + + if len(label) < 4 { + return r, errors.Errorf("label constraint should be in format '{+|-}key=value', but got '%s'", label) + } + + var op LabelConstraintOp + switch label[0] { + case '+': + op = In + case '-': + op = NotIn + default: + return r, errors.Errorf("label constraint should be in format '{+|-}key=value', but got '%s'", label) + } + + kv := strings.Split(label[1:], "=") + if len(kv) != 2 { + return r, errors.Errorf("label constraint should be in format '{+|-}key=value', but got '%s'", label) + } + + key := strings.TrimSpace(kv[0]) + if key == "" { + return r, errors.Errorf("label constraint should be in format '{+|-}key=value', but got '%s'", label) + } + + val := strings.TrimSpace(kv[1]) + if val == "" { + return r, errors.Errorf("label constraint should be in format '{+|-}key=value', but got '%s'", label) + } + + r.Key = key + r.Op = op + r.Values = []string{val} + return r, nil +} + +// CheckLabelConstraints will check labels, and build LabelConstraints for rule. +func CheckLabelConstraints(labels []string) ([]LabelConstraint, error) { + constraints := make([]LabelConstraint, 0, len(labels)) + for _, str := range labels { + label, err := checkLabelConstraint(strings.TrimSpace(str)) + if err != nil { + return constraints, err + } + constraints = append(constraints, label) + } + return constraints, nil +} diff --git a/ddl/placement_rule_test.go b/ddl/placement_rule_test.go index e1e6d29764..a4fa724fce 100644 --- a/ddl/placement_rule_test.go +++ b/ddl/placement_rule_test.go @@ -11,161 +11,212 @@ // See the License for the specific language governing permissions and // limitations under the License. -package ddl_test +package ddl import ( . "github.com/pingcap/check" - "github.com/pingcap/tidb/ddl" - "github.com/pingcap/tidb/util/testkit" + "github.com/pingcap/parser/ast" + "github.com/pingcap/tidb/ddl/placement" ) -func (s *testDBSuite1) TestAlterTableAlterPartition(c *C) { - tk := testkit.NewTestKit(c, s.store) - tk.MustExec("use test") - tk.MustExec("drop table if exists t1") - defer tk.MustExec("drop table if exists t1") +var _ = Suite(&testPlacementSuite{}) - tk.MustExec(`create table t1 (c int) -PARTITION BY RANGE (c) ( - PARTITION p0 VALUES LESS THAN (6), - PARTITION p1 VALUES LESS THAN (11), - PARTITION p2 VALUES LESS THAN (16), - PARTITION p3 VALUES LESS THAN (21) -);`) - - // normal cases - _, err := tk.Exec(`alter table t1 alter partition p0 -add placement policy - constraints='["+zone=sh"]' - role=leader - replicas=3`) - c.Assert(err, ErrorMatches, ".*pd unavailable.*") - - _, err = tk.Exec(`alter table t1 alter partition p0 -add placement policy - constraints='["+ zone = sh ", "- zone = bj "]' - role=leader - replicas=3`) - c.Assert(err, ErrorMatches, ".*pd unavailable.*") - - _, err = tk.Exec(`alter table t1 alter partition p0 -add placement policy - constraints='{"+ zone = sh ": 1}' - role=leader - replicas=3`) - c.Assert(err, ErrorMatches, ".*pd unavailable.*") - - _, err = tk.Exec(`alter table t1 alter partition p0 -add placement policy - constraints='{"+ zone = sh, -zone = bj ": 1}' - role=leader - replicas=3`) - c.Assert(err, ErrorMatches, ".*pd unavailable.*") - - _, err = tk.Exec(`alter table t1 alter partition p0 -add placement policy - constraints='{"+ zone = sh ": 1, "- zone = bj": 2}' - role=leader - replicas=3`) - c.Assert(err, ErrorMatches, ".*pd unavailable.*") - - // list/dict detection - _, err = tk.Exec(`alter table t1 alter partition p0 -add placement policy - constraints=',,,' - role=leader - replicas=3`) - c.Assert(err, ErrorMatches, ".*array or object.*") - - _, err = tk.Exec(`alter table t1 alter partition p0 -add placement policy - constraints='[,,,' - role=leader - replicas=3`) - c.Assert(err, ErrorMatches, ".*invalid character.*") - - _, err = tk.Exec(`alter table t1 alter partition p0 -add placement policy - constraints='{,,,' - role=leader - replicas=3`) - c.Assert(err, ErrorMatches, ".*invalid character.*") - - // checkPlacementSpecConstraint - _, err = tk.Exec(`alter table t1 alter partition p0 -add placement policy - constraints='[",,,"]' - role=leader - 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 - 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 - 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 - 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 - 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 - 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 - 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 - 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 - replicas=3`) - c.Assert(err, ErrorMatches, ".*Unknown partition.*") - - tk.MustExec("drop table if exists t1") - tk.MustExec("create table t1 (c int)") - - _, err = tk.Exec(`alter table t1 alter partition p -add placement policy - constraints='["+zone=sh"]' - role=leader - replicas=3`) - c.Assert(ddl.ErrPartitionMgmtOnNonpartitioned.Equal(err), IsTrue) +type testPlacementSuite struct { +} + +func (s *testPlacementSuite) TestPlacementBuild(c *C) { + tests := []struct { + input []*ast.PlacementSpec + output []*placement.RuleOp + err string + }{ + { + input: []*ast.PlacementSpec{}, + output: []*placement.RuleOp{}, + }, + { + input: []*ast.PlacementSpec{{ + Role: ast.PlacementRoleVoter, + Tp: ast.PlacementAdd, + Replicas: 3, + Constraints: `["+ zone=sh", "-zone = bj"]`, + }}, + output: []*placement.RuleOp{ + { + Action: placement.RuleOpAdd, + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + Role: placement.Voter, + Override: true, + Count: 3, + LabelConstraints: []placement.LabelConstraint{ + {Key: "zone", Op: "in", Values: []string{"sh"}}, + {Key: "zone", Op: "notIn", Values: []string{"bj"}}, + }, + }, + }}, + }, + { + input: []*ast.PlacementSpec{ + { + Role: ast.PlacementRoleVoter, + Tp: ast.PlacementAdd, + Replicas: 3, + Constraints: `["+ zone=sh", "-zone = bj"]`, + }, + { + Role: ast.PlacementRoleFollower, + Tp: ast.PlacementAdd, + Replicas: 2, + Constraints: `["- zone=sh", "+zone = bj"]`, + }, + }, + output: []*placement.RuleOp{ + { + Action: placement.RuleOpAdd, + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + Role: placement.Voter, + Override: true, + Count: 3, + LabelConstraints: []placement.LabelConstraint{ + {Key: "zone", Op: "in", Values: []string{"sh"}}, + {Key: "zone", Op: "notIn", Values: []string{"bj"}}, + }, + }, + }, + { + Action: placement.RuleOpAdd, + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + Role: placement.Follower, + Override: true, + Count: 2, + LabelConstraints: []placement.LabelConstraint{ + {Key: "zone", Op: "notIn", Values: []string{"sh"}}, + {Key: "zone", Op: "in", Values: []string{"bj"}}, + }, + }, + }, + }, + }, + { + input: []*ast.PlacementSpec{ + { + Role: ast.PlacementRoleVoter, + Tp: ast.PlacementAdd, + Replicas: 3, + Constraints: `["+ zone=sh", "-zone = bj"]`, + }, + { + Role: ast.PlacementRoleVoter, + Tp: ast.PlacementAlter, + Replicas: 2, + Constraints: `["- zone=sh", "+zone = bj"]`, + }, + }, + output: []*placement.RuleOp{ + { + Action: placement.RuleOpDel, + DeleteByIDPrefix: true, + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + Role: placement.Voter, + }, + }, + { + Action: placement.RuleOpAdd, + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + Role: placement.Voter, + Override: true, + Count: 2, + LabelConstraints: []placement.LabelConstraint{ + {Key: "zone", Op: "notIn", Values: []string{"sh"}}, + {Key: "zone", Op: "in", Values: []string{"bj"}}, + }, + }, + }, + }, + }, + { + input: []*ast.PlacementSpec{ + { + Role: ast.PlacementRoleVoter, + Tp: ast.PlacementAdd, + Replicas: 3, + Constraints: `["+ zone=sh", "-zone = bj"]`, + }, + { + Role: ast.PlacementRoleVoter, + Tp: ast.PlacementAlter, + Replicas: 3, + Constraints: `{"- zone=sh":1, "+zone = bj":1}`, + }, + }, + output: []*placement.RuleOp{ + { + Action: placement.RuleOpDel, + DeleteByIDPrefix: true, + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + Role: placement.Voter, + }, + }, + { + Action: placement.RuleOpAdd, + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + Role: placement.Voter, + Override: true, + Count: 1, + LabelConstraints: []placement.LabelConstraint{{Key: "zone", Op: "in", Values: []string{"bj"}}}, + }, + }, + { + Action: placement.RuleOpAdd, + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + Role: placement.Voter, + Override: true, + Count: 1, + LabelConstraints: []placement.LabelConstraint{{Key: "zone", Op: "notIn", Values: []string{"sh"}}}, + }, + }, + { + Action: placement.RuleOpAdd, + Rule: &placement.Rule{ + GroupID: placement.RuleDefaultGroupID, + Role: placement.Voter, + Override: true, + Count: 1, + }, + }, + }, + }, + } + for k, t := range tests { + out, err := buildPlacementSpecs(t.input) + c.Logf("test %d\n", k) + if err == nil { + for i := range t.output { + c.Logf("\t%d-th output\n", i) + found := false + for j := range out { + ok1, _ := DeepEquals.Check([]interface{}{out[j].Action, t.output[i].Action}, nil) + ok2, _ := DeepEquals.Check([]interface{}{out[j].DeleteByIDPrefix, t.output[i].DeleteByIDPrefix}, nil) + ok3, _ := DeepEquals.Check([]interface{}{out[j].Rule, t.output[i].Rule}, nil) + if ok1 && ok2 && ok3 { + found = true + break + } + } + if !found { + c.Fatalf("\texcept %+v, but got %+v\n", t.output[i], out) + } + } + } else { + c.Assert(err.Error(), ErrorMatches, t.err) + } + } } diff --git a/ddl/placement_sql_test.go b/ddl/placement_sql_test.go new file mode 100644 index 0000000000..49d1b2ef24 --- /dev/null +++ b/ddl/placement_sql_test.go @@ -0,0 +1,252 @@ +// Copyright 2020 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// See the License for the specific language governing permissions and +// limitations under the License. + +package ddl_test + +import ( + . "github.com/pingcap/check" + "github.com/pingcap/tidb/ddl" + "github.com/pingcap/tidb/util/testkit" +) + +func (s *testDBSuite1) TestAlterTableAlterPartition(c *C) { + tk := testkit.NewTestKit(c, s.store) + tk.MustExec("use test") + tk.MustExec("drop table if exists t1") + defer tk.MustExec("drop table if exists t1") + + tk.MustExec(`create table t1 (c int) +PARTITION BY RANGE (c) ( + PARTITION p0 VALUES LESS THAN (6), + PARTITION p1 VALUES LESS THAN (11), + PARTITION p2 VALUES LESS THAN (16), + PARTITION p3 VALUES LESS THAN (21) +);`) + + // normal cases + _, err := tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='["+zone=sh"]' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*pd unavailable.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='["+ zone = sh ", "- zone = bj "]' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*pd unavailable.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='{"+ zone = sh ": 1}' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*pd unavailable.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='{"+ zone = sh, -zone = bj ": 1}' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*pd unavailable.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='{"+ zone = sh ": 1, "- zone = bj": 2}' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*pd unavailable.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +alter placement policy + constraints='{"+ zone = sh, -zone = bj ": 1}' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*pd unavailable.*") + + // multiple statements + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='["+ zone = sh "]' + role=leader + replicas=3, +add placement policy + constraints='{"+ zone = sh, -zone = bj ": 1}' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*pd unavailable.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='["+ zone = sh "]' + role=leader + replicas=3, +add placement policy + constraints='{"+zone=sh,+zone=bj":1,"+zone=sh,+zone=bj":1}' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*pd unavailable.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='{"+ zone = sh ": 1, "- zone = bj,+zone=sh": 2}' + role=leader + replicas=3, +alter placement policy + constraints='{"+ zone = sh, -zone = bj ": 1}' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*pd unavailable.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='["+zone=sh", "-zone=bj"]' + role=leader + replicas=3, +add placement policy + constraints='{"+zone=sh": 1}' + role=leader + replicas=3, +add placement policy + constraints='{"+zone=sh,+zone=bj":1,"+zone=sh,+zone=bj":1}' + role=leader + replicas=3, +alter placement policy + constraints='{"+zone=sh": 1, "-zon =bj,+zone=sh": 1}' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*pd unavailable.*") + + // list/dict detection + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints=',,,' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*array or object.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='[,,,' + role=leader + replicas=3`) + c.Assert(err, ErrorMatches, ".*invalid character.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='{,,,' + role=leader + 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 + 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 + 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 + 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 + 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 + 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 + 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 + 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 + 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 + 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 + 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 + replicas=3`) + c.Assert(err, ErrorMatches, ".*count should be positive.*") + + _, err = tk.Exec(`alter table t1 alter partition p0 +add placement policy + constraints='["+ zone = sh"]' + role=leader + replicas=0`) + c.Assert(err, ErrorMatches, ".*Invalid placement option REPLICAS, it is not allowed to be 0.*") + + tk.MustExec("drop table if exists t1") + tk.MustExec("create table t1 (c int)") + + _, err = tk.Exec(`alter table t1 alter partition p +add placement policy + constraints='["+zone=sh"]' + role=leader + replicas=3`) + c.Assert(ddl.ErrPartitionMgmtOnNonpartitioned.Equal(err), IsTrue) +} diff --git a/domain/infosync/info.go b/domain/infosync/info.go index 170bf3a44d..bf2db81a43 100644 --- a/domain/infosync/info.go +++ b/domain/infosync/info.go @@ -32,8 +32,8 @@ import ( "github.com/pingcap/failpoint" "github.com/pingcap/parser/mysql" "github.com/pingcap/parser/terror" - "github.com/pingcap/pd/v4/server/schedule/placement" "github.com/pingcap/tidb/config" + "github.com/pingcap/tidb/ddl/placement" "github.com/pingcap/tidb/ddl/util" "github.com/pingcap/tidb/errno" "github.com/pingcap/tidb/kv" @@ -305,7 +305,11 @@ func doRequest(ctx context.Context, addrs []string, route, method string, body i } // UpdatePlacementRules is used to notify PD changes of placement rules. -func UpdatePlacementRules(ctx context.Context, rules []*placement.Rule) error { +func UpdatePlacementRules(ctx context.Context, rules []*placement.RuleOp) error { + if len(rules) == 0 { + return nil + } + is, err := getGlobalInfoSyncer() if err != nil { return err @@ -325,7 +329,7 @@ func UpdatePlacementRules(ctx context.Context, rules []*placement.Rule) error { return err } - err = doRequest(ctx, addrs, path.Join(pdapi.Config, "rules"), http.MethodPost, bytes.NewReader(b)) + err = doRequest(ctx, addrs, path.Join(pdapi.Config, "rules/batch"), http.MethodPost, bytes.NewReader(b)) if err != nil { return err } diff --git a/go.mod b/go.mod index 580734ae48..5bb46d1ee9 100644 --- a/go.mod +++ b/go.mod @@ -36,7 +36,7 @@ require ( github.com/pingcap/goleveldb v0.0.0-20191226122134-f82aafb29989 github.com/pingcap/kvproto v0.0.0-20200803054707-ebd5de15093f github.com/pingcap/log v0.0.0-20200511115504-543df19646ad - github.com/pingcap/parser v0.0.0-20200731033026-84f62115187c + github.com/pingcap/parser v0.0.0-20200810083003-f0edf4170630 github.com/pingcap/pd/v4 v4.0.0-rc.2.0.20200730093003-dc8c75cf7ca0 github.com/pingcap/sysutil v0.0.0-20200715082929-4c47bcac246a github.com/pingcap/tidb-tools v4.0.1+incompatible diff --git a/go.sum b/go.sum index 61df0750a5..4259310867 100644 --- a/go.sum +++ b/go.sum @@ -476,6 +476,8 @@ github.com/pingcap/parser v0.0.0-20200623082809-b74301ac298b/go.mod h1:vQdbJqobJ github.com/pingcap/parser v0.0.0-20200730092557-34a468e9b774/go.mod h1:vQdbJqobJAgFyiRNNtXahpMoGWwPEuWciVEK5A20NS0= github.com/pingcap/parser v0.0.0-20200731033026-84f62115187c h1:UG7JwE9UbEpiTOJeUXHQBJFfMrU+yAtdpzEbLRiAnkI= github.com/pingcap/parser v0.0.0-20200731033026-84f62115187c/go.mod h1:vQdbJqobJAgFyiRNNtXahpMoGWwPEuWciVEK5A20NS0= +github.com/pingcap/parser v0.0.0-20200810083003-f0edf4170630 h1:65ZVh0OO2/y7QeocVX+6bfOxVYzTkmQWyj7zazUpwBg= +github.com/pingcap/parser v0.0.0-20200810083003-f0edf4170630/go.mod h1:vQdbJqobJAgFyiRNNtXahpMoGWwPEuWciVEK5A20NS0= github.com/pingcap/pd/v4 v4.0.0-rc.1.0.20200422143320-428acd53eba2/go.mod h1:s+utZtXDznOiL24VK0qGmtoHjjXNsscJx3m1n8cC56s= github.com/pingcap/pd/v4 v4.0.0-rc.2.0.20200520083007-2c251bd8f181/go.mod h1:q4HTx/bA8aKBa4S7L+SQKHvjRPXCRV0tA0yRw0qkZSA= github.com/pingcap/pd/v4 v4.0.0-rc.2.0.20200714122454-1a64f969cb3c h1:aOiyGetA256/LUkdmhny0Q/PWTBQiF/TPNhJuJMGRSY= diff --git a/util/testleak/leaktest.go b/util/testleak/leaktest.go index 994a3edd76..93115a5561 100644 --- a/util/testleak/leaktest.go +++ b/util/testleak/leaktest.go @@ -59,8 +59,6 @@ func interestingGoroutines() (gs []string) { "github.com/pingcap/goleveldb/leveldb.(*DB).compactionError", "github.com/pingcap/goleveldb/leveldb.(*DB).mpoolDrain", "go.etcd.io/etcd/v3/pkg/logutil.(*MergeLogger).outputLoop", - // import PD will introduce another MergeLogger - "go.etcd.io/etcd/pkg/logutil.(*MergeLogger).outputLoop", "oracles.(*pdOracle).updateTS", "tikv.(*tikvStore).runSafePointChecker", "tikv.(*RegionCache).asyncCheckAndResolveLoop",