From d8bcc2a375d75be7837587883af2c84567ea6e89 Mon Sep 17 00:00:00 2001 From: nolouch Date: Wed, 30 Aug 2023 19:19:07 +0800 Subject: [PATCH 1/7] placement: merge the rules if the constraints are the same Signed-off-by: nolouch --- ddl/placement/BUILD.bazel | 2 +- ddl/placement/bundle.go | 108 +++++++++++- ddl/placement/bundle_test.go | 326 ++++++++++++++++++++++++++++++++++- ddl/placement/constraints.go | 34 ++++ 4 files changed, 453 insertions(+), 17 deletions(-) diff --git a/ddl/placement/BUILD.bazel b/ddl/placement/BUILD.bazel index 761250c9c9064..8b1bb8532d240 100644 --- a/ddl/placement/BUILD.bazel +++ b/ddl/placement/BUILD.bazel @@ -36,7 +36,7 @@ go_test( embed = [":placement"], flaky = True, race = "on", - shard_count = 24, + shard_count = 25, deps = [ "//kv", "//meta", diff --git a/ddl/placement/bundle.go b/ddl/placement/bundle.go index 62faeb5fe8a3a..89b89546ebc31 100644 --- a/ddl/placement/bundle.go +++ b/ddl/placement/bundle.go @@ -289,7 +289,7 @@ func (b *Bundle) String() string { // Tidy will post optimize Rules, trying to generate rules that suits PD. func (b *Bundle) Tidy() error { extraCnt := map[PeerRoleType]int{} - newRules := b.Rules[:0] + tempRules := b.Rules[:0] // One Bundle is from one PlacementSettings, rule share same location labels, so we can use the first rule's location labels. var locationLabels []string @@ -299,7 +299,8 @@ func (b *Bundle) Tidy() error { break } } - for i, rule := range b.Rules { + id := 0 + for _, rule := range b.Rules { // useless Rule if rule.Count <= 0 { continue @@ -328,13 +329,15 @@ func (b *Bundle) Tidy() error { extraCnt[rule.Role] += rule.Count continue } - rule.ID = strconv.Itoa(i) - newRules = append(newRules, rule) + rule.ID = strconv.Itoa(id) + tempRules = append(tempRules, rule) + id++ } + for role, cnt := range extraCnt { // add -engine=tiflash, refer to tidb#22065. - newRules = append(newRules, &Rule{ - ID: string(role), + tempRules = append(tempRules, &Rule{ + ID: strconv.Itoa(id), Role: role, Count: cnt, Constraints: []Constraint{{ @@ -345,11 +348,102 @@ func (b *Bundle) Tidy() error { // the merged rule should have the same location labels with the original rules. LocationLabels: locationLabels, }) + id++ + } + groups := make(map[string]*ConstraintsGroup) + finalRules := tempRules[:0] + for _, rule := range tempRules { + key := rule.Constraints.FingerPrint() + existing, ok := groups[key] + if !ok { + groups[key] = &ConstraintsGroup{rules: []*Rule{rule}} + continue + } + existing.rules = append(existing.rules, rule) } - b.Rules = newRules + for _, group := range groups { + group.MergeRulesByRole() + } + if len(groups) == 1 { + for _, group := range groups { + group.MergeTransformableRoles() + } + } + for _, group := range groups { + finalRules = append(finalRules, group.rules...) + } + // sort by id + sort.SliceStable(finalRules, func(i, j int) bool { + return finalRules[i].ID < finalRules[j].ID + }) + b.Rules = finalRules return nil } +// ConstraintsGroup is a group of rules with the same constraints. +type ConstraintsGroup struct { + rules []*Rule +} + +// MergeRulesByRole merges the rules with the same role. +func (c *ConstraintsGroup) MergeRulesByRole() { + // Create a map to store rules by role + rulesByRole := make(map[PeerRoleType][]*Rule) + + // Iterate through each rule + for _, rule := range c.rules { + // Add the rule to the map based on its role + rulesByRole[rule.Role] = append(rulesByRole[rule.Role], rule) + } + + // Clear existing rules + c.rules = nil + + // Iterate through each role and merge the rules + for _, rules := range rulesByRole { + mergedRule := rules[0] + for i, rule := range rules { + if i == 0 { + continue + } + mergedRule.Count += rule.Count + if mergedRule.ID > rule.ID { + mergedRule.ID = rule.ID + } + } + c.rules = append(c.rules, mergedRule) + } +} + +// MergeTransformableRoles merges all the rules to one that can be transformed to other roles. +func (c *ConstraintsGroup) MergeTransformableRoles() { + if len(c.rules) == 0 { + return + } + newRules := make([]*Rule, 0, len(c.rules)) + var mergedRule *Rule + for _, rule := range c.rules { + // Learner is not transformable, it should be promote by PD. + if rule.Role == Learner { + newRules = append(newRules, rule) + continue + } + if mergedRule == nil { + mergedRule = rule + continue + } + mergedRule.Count += rule.Count + if mergedRule.ID > rule.ID { + mergedRule.ID = rule.ID + } + } + mergedRule.Role = Voter + if mergedRule != nil { + newRules = append(newRules, mergedRule) + } + c.rules = newRules +} + // Reset resets the bundle ID and keyrange of all rules. func (b *Bundle) Reset(ruleIndex int, newIDs []int64) *Bundle { // eliminate the redundant rules. diff --git a/ddl/placement/bundle_test.go b/ddl/placement/bundle_test.go index 0dc9032da8562..4c6aa41d8817c 100644 --- a/ddl/placement/bundle_test.go +++ b/ddl/placement/bundle_test.go @@ -17,6 +17,7 @@ package placement import ( "encoding/hex" "fmt" + "reflect" "testing" "github.com/pingcap/failpoint" @@ -861,17 +862,17 @@ func TestTidy(t *testing.T) { bundle.Rules = append(bundle.Rules, rules1...) bundle.Rules = append(bundle.Rules, rules2...) + require.Len(t, bundle.Rules, 3) err = bundle.Tidy() require.NoError(t, err) - require.Len(t, bundle.Rules, 2) - require.Equal(t, "1", bundle.Rules[0].ID) + require.Len(t, bundle.Rules, 1) + require.Equal(t, "0", bundle.Rules[0].ID) require.Len(t, bundle.Rules[0].Constraints, 3) require.Equal(t, Constraint{ Op: NotIn, Key: EngineLabelKey, Values: []string{EngineLabelTiFlash}, }, bundle.Rules[0].Constraints[2]) - require.Equal(t, "2", bundle.Rules[1].ID) // merge rules3, err := NewRules(Follower, 4, "") @@ -892,21 +893,21 @@ func TestTidy(t *testing.T) { } chkfunc := func() { require.NoError(t, err) - require.Len(t, bundle.Rules, 3) + require.Len(t, bundle.Rules, 2) require.Equal(t, "0", bundle.Rules[0].ID) require.Equal(t, "1", bundle.Rules[1].ID) - require.Equal(t, "follower", bundle.Rules[2].ID) - require.Equal(t, 9, bundle.Rules[2].Count) + require.Equal(t, 9, bundle.Rules[1].Count) require.Equal(t, Constraints{ { Op: NotIn, Key: EngineLabelKey, Values: []string{EngineLabelTiFlash}, }, - }, bundle.Rules[2].Constraints) - require.Equal(t, []string{"zone", "host"}, bundle.Rules[2].LocationLabels) + }, bundle.Rules[1].Constraints) + require.Equal(t, []string{"zone", "host"}, bundle.Rules[1].LocationLabels) } err = bundle.Tidy() + fmt.Println(bundle.String()) chkfunc() // tidy again @@ -921,10 +922,317 @@ func TestTidy(t *testing.T) { require.NoError(t, err) require.Equal(t, bundle, bundle2) - bundle.Rules[2].Constraints = append(bundle.Rules[2].Constraints, Constraint{ + bundle.Rules[1].Constraints = append(bundle.Rules[1].Constraints, Constraint{ Op: In, Key: EngineLabelKey, Values: []string{EngineLabelTiFlash}, }) require.ErrorIs(t, bundle.Tidy(), ErrConflictingConstraints) } + +func TestTidy2(t *testing.T) { + tests := []struct { + name string + bundle Bundle + expected Bundle + }{ + { + name: "Empty bundle", + bundle: Bundle{ + Rules: []*Rule{}, + }, + expected: Bundle{ + Rules: []*Rule{}, + }, + }, + { + name: "Rules with empty constraints are merged", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Count: 1, + Constraints: Constraints{}, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Voter, + Count: 2, + Constraints: Constraints{}, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Count: 3, + Constraints: Constraints{}, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints are merged, Leader + Follower", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 2, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 3, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints are merged, Leader + Voter", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 2, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 3, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints and role are merged, Leader + Follower + Voter", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 3, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints and role are merged, Leader + Follower + Voter + Learner", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "4", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 2, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 3, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 2, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with different constraints are kept separate", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "1", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.bundle.Tidy() + require.NoError(t, err) + + require.Equal(t, len(tt.expected.Rules), len(tt.bundle.Rules)) + + for i, rule := range tt.bundle.Rules { + expectedRule := tt.expected.Rules[i] + // Tiflash is always excluded from the constraints. + expectedRule.Constraints.Add(Constraint{ + Op: NotIn, + Key: EngineLabelKey, + Values: []string{EngineLabelTiFlash}, + }) + if !reflect.DeepEqual(rule, expectedRule) { + t.Errorf("unexpected rule at index %d:\nactual=%#v,\nexpected=%#v\n", i, rule, expectedRule) + } + } + }) + } +} diff --git a/ddl/placement/constraints.go b/ddl/placement/constraints.go index 38b678857eee3..9d2106f9a35a5 100644 --- a/ddl/placement/constraints.go +++ b/ddl/placement/constraints.go @@ -15,7 +15,10 @@ package placement import ( + "crypto/sha256" + "encoding/base64" "fmt" + "sort" "strings" "gopkg.in/yaml.v2" @@ -110,3 +113,34 @@ func (constraints *Constraints) Add(label Constraint) error { } return nil } + +// FingerPrint returns a unique string for the constraints. +func (constraints Constraints) FingerPrint() string { + copied := make(Constraints, len(constraints)) + copy(copied, constraints) + sort.SliceStable(copied, func(i, j int) bool { + return constraintToString(copied[i]) < constraintToString(copied[j]) + }) + + var combinedConstraints string + for _, constraint := range copied { + combinedConstraints += constraintToString(constraint) + } + + // Calculate the SHA256 hash of the concatenated constraints + hash := sha256.Sum256([]byte(combinedConstraints)) + + // Encode the hash as a base64 string + hashStr := base64.StdEncoding.EncodeToString(hash[:]) + + return hashStr +} + +func constraintToString(c Constraint) string { + // Sort the values in the constraint + sortedValues := make([]string, len(c.Values)) + copy(sortedValues, c.Values) + sort.Strings(sortedValues) + sortedValuesStr := strings.Join(sortedValues, ",") + return c.Key + "|" + string(c.Op) + "|" + sortedValuesStr +} From ff3a91d760f90a40c8d81b5479314654f22854a9 Mon Sep 17 00:00:00 2001 From: nolouch Date: Thu, 31 Aug 2023 15:38:06 +0800 Subject: [PATCH 2/7] remove duplicated code Signed-off-by: nolouch --- ddl/placement/bundle.go | 41 +---------------------------------------- 1 file changed, 1 insertion(+), 40 deletions(-) diff --git a/ddl/placement/bundle.go b/ddl/placement/bundle.go index 89b89546ebc31..4559458980945 100644 --- a/ddl/placement/bundle.go +++ b/ddl/placement/bundle.go @@ -288,28 +288,13 @@ func (b *Bundle) String() string { // Tidy will post optimize Rules, trying to generate rules that suits PD. func (b *Bundle) Tidy() error { - extraCnt := map[PeerRoleType]int{} tempRules := b.Rules[:0] - - // One Bundle is from one PlacementSettings, rule share same location labels, so we can use the first rule's location labels. - var locationLabels []string - for _, rule := range b.Rules { - if len(rule.LocationLabels) > 0 { - locationLabels = rule.LocationLabels - break - } - } id := 0 for _, rule := range b.Rules { // useless Rule if rule.Count <= 0 { continue } - // merge all empty constraints - if len(rule.Constraints) == 0 { - extraCnt[rule.Role] += rule.Count - continue - } // refer to tidb#22065. // add -engine=tiflash to every rule to avoid schedules to tiflash instances. // placement rules in SQL is not compatible with `set tiflash replica` yet @@ -321,35 +306,11 @@ func (b *Bundle) Tidy() error { if err != nil { return err } - // Constraints.Add() will automatically avoid duplication - // if -engine=tiflash is added and there is only one constraint - // then it must be -engine=tiflash - // it is seen as an empty constraint, so merge it - if len(rule.Constraints) == 1 { - extraCnt[rule.Role] += rule.Count - continue - } rule.ID = strconv.Itoa(id) tempRules = append(tempRules, rule) id++ } - for role, cnt := range extraCnt { - // add -engine=tiflash, refer to tidb#22065. - tempRules = append(tempRules, &Rule{ - ID: strconv.Itoa(id), - Role: role, - Count: cnt, - Constraints: []Constraint{{ - Op: NotIn, - Key: EngineLabelKey, - Values: []string{EngineLabelTiFlash}, - }}, - // the merged rule should have the same location labels with the original rules. - LocationLabels: locationLabels, - }) - id++ - } groups := make(map[string]*ConstraintsGroup) finalRules := tempRules[:0] for _, rule := range tempRules { @@ -437,8 +398,8 @@ func (c *ConstraintsGroup) MergeTransformableRoles() { mergedRule.ID = rule.ID } } - mergedRule.Role = Voter if mergedRule != nil { + mergedRule.Role = Voter newRules = append(newRules, mergedRule) } c.rules = newRules From f864dda739316a6b7b18b38f40c315e5beea83d5 Mon Sep 17 00:00:00 2001 From: nolouch Date: Sun, 10 Sep 2023 15:51:10 +0800 Subject: [PATCH 3/7] add cases Signed-off-by: nolouch --- ddl/placement/bundle.go | 36 +++++++- ddl/placement/bundle_test.go | 158 ++++++++++++++++++++++++++++++++++- 2 files changed, 192 insertions(+), 2 deletions(-) diff --git a/ddl/placement/bundle.go b/ddl/placement/bundle.go index 4559458980945..ed3abcdefafe3 100644 --- a/ddl/placement/bundle.go +++ b/ddl/placement/bundle.go @@ -329,6 +329,8 @@ func (b *Bundle) Tidy() error { for _, group := range groups { group.MergeTransformableRoles() } + } else { + transformableLeaderConstraint(groups) } for _, group := range groups { finalRules = append(finalRules, group.rules...) @@ -343,7 +345,30 @@ func (b *Bundle) Tidy() error { // ConstraintsGroup is a group of rules with the same constraints. type ConstraintsGroup struct { - rules []*Rule + rules []*Rule + canBecameLeader bool + isLeaderGroup bool +} + +func transformableLeaderConstraint(groups map[string]*ConstraintsGroup) error { + var leaderGroup *ConstraintsGroup + canBecameLeaderNum := 0 + for _, group := range groups { + if group.isLeaderGroup { + if leaderGroup == nil { + leaderGroup = group + } else { + return errors.New("multiple leader group") + } + } + if group.canBecameLeader { + canBecameLeaderNum++ + } + } + if leaderGroup != nil && canBecameLeaderNum == 1 { + leaderGroup.MergeTransformableRoles() + } + return nil } // MergeRulesByRole merges the rules with the same role. @@ -355,6 +380,12 @@ func (c *ConstraintsGroup) MergeRulesByRole() { for _, rule := range c.rules { // Add the rule to the map based on its role rulesByRole[rule.Role] = append(rulesByRole[rule.Role], rule) + if rule.Role == Leader || rule.Role == Voter { + c.canBecameLeader = true + } + if rule.Role == Leader { + c.isLeaderGroup = true + } } // Clear existing rules @@ -383,6 +414,9 @@ func (c *ConstraintsGroup) MergeTransformableRoles() { } newRules := make([]*Rule, 0, len(c.rules)) var mergedRule *Rule + if len(c.rules) == 1 { + return + } for _, rule := range c.rules { // Learner is not transformable, it should be promote by PD. if rule.Role == Learner { diff --git a/ddl/placement/bundle_test.go b/ddl/placement/bundle_test.go index 4c6aa41d8817c..0d4837097c8a0 100644 --- a/ddl/placement/bundle_test.go +++ b/ddl/placement/bundle_test.go @@ -907,7 +907,6 @@ func TestTidy(t *testing.T) { require.Equal(t, []string{"zone", "host"}, bundle.Rules[1].LocationLabels) } err = bundle.Tidy() - fmt.Println(bundle.String()) chkfunc() // tidy again @@ -1165,6 +1164,163 @@ func TestTidy2(t *testing.T) { }, }, }, + { + name: "Rules with same constraints and role are merged, Leader + Follower + Learner | Follower", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "4", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "0", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 2, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + }, + { + name: "Rules with same constraints and role are merged, Leader + Follower + Learner | Voter", + bundle: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "4", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + expected: Bundle{ + Rules: []*Rule{ + { + ID: "1", + Role: Leader, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "2", + Role: Follower, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "3", + Role: Learner, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"1"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + { + ID: "4", + Role: Voter, + Constraints: Constraints{ + {Op: In, Key: "rack", Values: []string{"2"}}, + }, + Count: 1, + LocationLabels: []string{"region"}, + }, + }, + }, + }, { name: "Rules with different constraints are kept separate", bundle: Bundle{ From 7bdae4f4ccdeae1eb2e41c198fb2b9bb0bf8a696 Mon Sep 17 00:00:00 2001 From: nolouch Date: Sun, 10 Sep 2023 23:11:59 +0800 Subject: [PATCH 4/7] address Signed-off-by: nolouch --- ddl/placement/bundle.go | 11 ++++++----- ddl/placement/bundle_test.go | 8 ++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/ddl/placement/bundle.go b/ddl/placement/bundle.go index 6bae8bc97e525..4d43e07d86f4c 100644 --- a/ddl/placement/bundle.go +++ b/ddl/placement/bundle.go @@ -330,7 +330,9 @@ func (b *Bundle) Tidy() error { group.MergeTransformableRoles() } } else { - transformableLeaderConstraint(groups) + if err := transformableLeaderConstraint(groups); err != nil { + return err + } } for _, group := range groups { finalRules = append(finalRules, group.rules...) @@ -355,11 +357,10 @@ func transformableLeaderConstraint(groups map[string]*ConstraintsGroup) error { canBecameLeaderNum := 0 for _, group := range groups { if group.isLeaderGroup { - if leaderGroup == nil { - leaderGroup = group - } else { - return errors.New("multiple leader group") + if !(leaderGroup == nil) { + return ErrInvalidPlacementOptions } + leaderGroup = group } if group.canBecameLeader { canBecameLeaderNum++ diff --git a/ddl/placement/bundle_test.go b/ddl/placement/bundle_test.go index 0d4837097c8a0..918c2cf18daac 100644 --- a/ddl/placement/bundle_test.go +++ b/ddl/placement/bundle_test.go @@ -1283,7 +1283,7 @@ func TestTidy2(t *testing.T) { expected: Bundle{ Rules: []*Rule{ { - ID: "1", + ID: "0", Role: Leader, Constraints: Constraints{ {Op: In, Key: "rack", Values: []string{"1"}}, @@ -1292,7 +1292,7 @@ func TestTidy2(t *testing.T) { LocationLabels: []string{"region"}, }, { - ID: "2", + ID: "1", Role: Follower, Constraints: Constraints{ {Op: In, Key: "rack", Values: []string{"1"}}, @@ -1301,7 +1301,7 @@ func TestTidy2(t *testing.T) { LocationLabels: []string{"region"}, }, { - ID: "3", + ID: "2", Role: Learner, Constraints: Constraints{ {Op: In, Key: "rack", Values: []string{"1"}}, @@ -1310,7 +1310,7 @@ func TestTidy2(t *testing.T) { LocationLabels: []string{"region"}, }, { - ID: "4", + ID: "3", Role: Voter, Constraints: Constraints{ {Op: In, Key: "rack", Values: []string{"2"}}, From c31db43dacbfc591d42bb270ed23ebd886eb69ac Mon Sep 17 00:00:00 2001 From: nolouch Date: Mon, 11 Sep 2023 15:26:55 +0800 Subject: [PATCH 5/7] address Signed-off-by: nolouch --- ddl/placement/constraints.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ddl/placement/constraints.go b/ddl/placement/constraints.go index 9d2106f9a35a5..4cd0a6c7496e9 100644 --- a/ddl/placement/constraints.go +++ b/ddl/placement/constraints.go @@ -15,9 +15,11 @@ package placement import ( + "cmp" "crypto/sha256" "encoding/base64" "fmt" + "slices" "sort" "strings" @@ -118,10 +120,10 @@ func (constraints *Constraints) Add(label Constraint) error { func (constraints Constraints) FingerPrint() string { copied := make(Constraints, len(constraints)) copy(copied, constraints) - sort.SliceStable(copied, func(i, j int) bool { - return constraintToString(copied[i]) < constraintToString(copied[j]) + slices.SortStableFunc(copied, func(i, j Constraint) int { + a, b := constraintToString(i), constraintToString(j) + return cmp.Compare(a, b) }) - var combinedConstraints string for _, constraint := range copied { combinedConstraints += constraintToString(constraint) From cf96e6eb275e5115eb8ddded0662e2ceb6b6f350 Mon Sep 17 00:00:00 2001 From: nolouch Date: Fri, 22 Sep 2023 15:31:25 +0800 Subject: [PATCH 6/7] address Signed-off-by: nolouch --- ddl/placement/bundle.go | 47 ++++++++++++++++++------------------ ddl/placement/constraints.go | 12 ++++----- 2 files changed, 29 insertions(+), 30 deletions(-) diff --git a/ddl/placement/bundle.go b/ddl/placement/bundle.go index 4d43e07d86f4c..de9f8546a7744 100644 --- a/ddl/placement/bundle.go +++ b/ddl/placement/bundle.go @@ -311,13 +311,13 @@ func (b *Bundle) Tidy() error { id++ } - groups := make(map[string]*ConstraintsGroup) + groups := make(map[string]*constraintsGroup) finalRules := tempRules[:0] for _, rule := range tempRules { key := rule.Constraints.FingerPrint() existing, ok := groups[key] if !ok { - groups[key] = &ConstraintsGroup{rules: []*Rule{rule}} + groups[key] = &constraintsGroup{rules: []*Rule{rule}} continue } existing.rules = append(existing.rules, rule) @@ -325,14 +325,8 @@ func (b *Bundle) Tidy() error { for _, group := range groups { group.MergeRulesByRole() } - if len(groups) == 1 { - for _, group := range groups { - group.MergeTransformableRoles() - } - } else { - if err := transformableLeaderConstraint(groups); err != nil { - return err - } + if err := transformableLeaderConstraint(groups); err != nil { + return err } for _, group := range groups { finalRules = append(finalRules, group.rules...) @@ -345,19 +339,22 @@ func (b *Bundle) Tidy() error { return nil } -// ConstraintsGroup is a group of rules with the same constraints. -type ConstraintsGroup struct { - rules []*Rule +// constraintsGroup is a group of rules with the same constraints. +type constraintsGroup struct { + rules []*Rule + // canBecameLeader means the group has leader/voter role, + // it's valid if it has leader. canBecameLeader bool - isLeaderGroup bool + // isLeaderGroup means it has specified leader role in this group. + isLeaderGroup bool } -func transformableLeaderConstraint(groups map[string]*ConstraintsGroup) error { - var leaderGroup *ConstraintsGroup +func transformableLeaderConstraint(groups map[string]*constraintsGroup) error { + var leaderGroup *constraintsGroup canBecameLeaderNum := 0 for _, group := range groups { if group.isLeaderGroup { - if !(leaderGroup == nil) { + if leaderGroup != nil { return ErrInvalidPlacementOptions } leaderGroup = group @@ -366,6 +363,11 @@ func transformableLeaderConstraint(groups map[string]*ConstraintsGroup) error { canBecameLeaderNum++ } } + // If there is a specified group should have leader, and only this group can be a leader, that means + // the leader's priority is certain, so we can merge the transformable rules into one. + // eg: + // - [ group1 (L F), group2 (F) ], after merging is [group1 (2*V), group2 (F)], we still know the leader prefers group1. + // - [ group1(L, F), group2(V) ], after merging is [group1 (2*V), group2 (V)], we can't know leader priority after merge. if leaderGroup != nil && canBecameLeaderNum == 1 { leaderGroup.MergeTransformableRoles() } @@ -373,7 +375,7 @@ func transformableLeaderConstraint(groups map[string]*ConstraintsGroup) error { } // MergeRulesByRole merges the rules with the same role. -func (c *ConstraintsGroup) MergeRulesByRole() { +func (c *constraintsGroup) MergeRulesByRole() { // Create a map to store rules by role rulesByRole := make(map[PeerRoleType][]*Rule) @@ -409,15 +411,12 @@ func (c *ConstraintsGroup) MergeRulesByRole() { } // MergeTransformableRoles merges all the rules to one that can be transformed to other roles. -func (c *ConstraintsGroup) MergeTransformableRoles() { - if len(c.rules) == 0 { +func (c *constraintsGroup) MergeTransformableRoles() { + if len(c.rules) == 0 || len(c.rules) == 1 { return } - newRules := make([]*Rule, 0, len(c.rules)) var mergedRule *Rule - if len(c.rules) == 1 { - return - } + newRules := make([]*Rule, 0, len(c.rules)) for _, rule := range c.rules { // Learner is not transformable, it should be promote by PD. if rule.Role == Learner { diff --git a/ddl/placement/constraints.go b/ddl/placement/constraints.go index 4cd0a6c7496e9..99467a5fc7f36 100644 --- a/ddl/placement/constraints.go +++ b/ddl/placement/constraints.go @@ -117,16 +117,16 @@ func (constraints *Constraints) Add(label Constraint) error { } // FingerPrint returns a unique string for the constraints. -func (constraints Constraints) FingerPrint() string { - copied := make(Constraints, len(constraints)) - copy(copied, constraints) +func (constraints *Constraints) FingerPrint() string { + copied := make(Constraints, len(*constraints)) + copy(copied, *constraints) slices.SortStableFunc(copied, func(i, j Constraint) int { - a, b := constraintToString(i), constraintToString(j) + a, b := constraintToString(&i), constraintToString(&j) return cmp.Compare(a, b) }) var combinedConstraints string for _, constraint := range copied { - combinedConstraints += constraintToString(constraint) + combinedConstraints += constraintToString(&constraint) } // Calculate the SHA256 hash of the concatenated constraints @@ -138,7 +138,7 @@ func (constraints Constraints) FingerPrint() string { return hashStr } -func constraintToString(c Constraint) string { +func constraintToString(c *Constraint) string { // Sort the values in the constraint sortedValues := make([]string, len(c.Values)) copy(sortedValues, c.Values) From 2d9de7203c2cf17068daf8fd8ed407b47a9c34dc Mon Sep 17 00:00:00 2001 From: nolouch Date: Mon, 25 Sep 2023 14:49:44 +0800 Subject: [PATCH 7/7] fmt Signed-off-by: nolouch --- ddl/placement/bundle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddl/placement/bundle.go b/ddl/placement/bundle.go index de9f8546a7744..28b3a0999dfd9 100644 --- a/ddl/placement/bundle.go +++ b/ddl/placement/bundle.go @@ -367,7 +367,7 @@ func transformableLeaderConstraint(groups map[string]*constraintsGroup) error { // the leader's priority is certain, so we can merge the transformable rules into one. // eg: // - [ group1 (L F), group2 (F) ], after merging is [group1 (2*V), group2 (F)], we still know the leader prefers group1. - // - [ group1(L, F), group2(V) ], after merging is [group1 (2*V), group2 (V)], we can't know leader priority after merge. + // - [ group1 (L F), group2 (V) ], after merging is [group1 (2*V), group2 (V)], we can't know leader priority after merge. if leaderGroup != nil && canBecameLeaderNum == 1 { leaderGroup.MergeTransformableRoles() }