From d8bcc2a375d75be7837587883af2c84567ea6e89 Mon Sep 17 00:00:00 2001 From: nolouch Date: Wed, 30 Aug 2023 19:19:07 +0800 Subject: [PATCH] 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 +}