From 9fb5f6df65d0d4e7d0fccc5371ab428d7e4df7a9 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Tue, 31 Dec 2024 18:35:01 +0800 Subject: [PATCH 1/2] Remove explicit exclude for "engine" notIn "tiflash" Signed-off-by: JaySon-Huang --- pkg/ddl/placement/bundle.go | 14 +++--------- pkg/ddl/placement/bundle_test.go | 33 +++++----------------------- pkg/ddl/placement/constraint.go | 1 + pkg/ddl/placement/constraint_test.go | 9 ++++++++ 4 files changed, 18 insertions(+), 39 deletions(-) diff --git a/pkg/ddl/placement/bundle.go b/pkg/ddl/placement/bundle.go index ee84ddd3c00f7..0dbb985af86b0 100644 --- a/pkg/ddl/placement/bundle.go +++ b/pkg/ddl/placement/bundle.go @@ -314,6 +314,9 @@ func (b *Bundle) String() string { // Tidy will post optimize Rules, trying to generate rules that suits PD. func (b *Bundle) Tidy() error { + // refer to #58633 + // Note that does not explicitly set rule with label.key==EngineLabelKey, because the + // PD may wrongly add peer to the unexpected stores if that key is specified. tempRules := b.Rules[:0] id := 0 for _, rule := range b.Rules { @@ -321,17 +324,6 @@ func (b *Bundle) Tidy() error { if rule.Count <= 0 { 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 - err := AddConstraint(&rule.LabelConstraints, pd.LabelConstraint{ - Op: pd.NotIn, - Key: EngineLabelKey, - Values: []string{EngineLabelTiFlash}, - }) - if err != nil { - return err - } rule.ID = strconv.Itoa(id) tempRules = append(tempRules, rule) id++ diff --git a/pkg/ddl/placement/bundle_test.go b/pkg/ddl/placement/bundle_test.go index 204ca23ccdc6c..2991ed996ee37 100644 --- a/pkg/ddl/placement/bundle_test.go +++ b/pkg/ddl/placement/bundle_test.go @@ -347,9 +347,10 @@ func TestString(t *testing.T) { require.NoError(t, err) rules2, err := newRules(pd.Voter, 4, `["-zone=sh", "+zone=bj"]`) require.NoError(t, err) - bundle.Rules = append(rules1, rules2...) + rules3, err := newRules(pd.Voter, 3, `["-engine=tiflash", "-engine=tiflash_compute"]`) + bundle.Rules = append(append(rules1, rules2...), rules3...) - require.Equal(t, "{\"group_id\":\"TiDB_DDL_1\",\"group_index\":0,\"group_override\":false,\"rules\":[{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":3,\"label_constraints\":[{\"key\":\"zone\",\"op\":\"in\",\"values\":[\"sh\"]}]},{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":4,\"label_constraints\":[{\"key\":\"zone\",\"op\":\"notIn\",\"values\":[\"sh\"]},{\"key\":\"zone\",\"op\":\"in\",\"values\":[\"bj\"]}]}]}", bundle.String()) + require.Equal(t, "{\"group_id\":\"TiDB_DDL_1\",\"group_index\":0,\"group_override\":false,\"rules\":[{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":3,\"label_constraints\":[{\"key\":\"zone\",\"op\":\"in\",\"values\":[\"sh\"]}]},{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":4,\"label_constraints\":[{\"key\":\"zone\",\"op\":\"notIn\",\"values\":[\"sh\"]},{\"key\":\"zone\",\"op\":\"in\",\"values\":[\"bj\"]}]},{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":3,\"label_constraints\":[{\"key\":\"engine\",\"op\":\"notIn\",\"values\":[\"tiflash\"]},{\"key\":\"engine\",\"op\":\"notIn\",\"values\":[\"tiflash_compute\"]}]}]}", bundle.String()) require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/pkg/ddl/placement/MockMarshalFailure", `return(true)`)) defer func() { @@ -956,12 +957,7 @@ func TestTidy(t *testing.T) { require.NoError(t, err) require.Len(t, bundle.Rules, 1) require.Equal(t, "0", bundle.Rules[0].ID) - require.Len(t, bundle.Rules[0].LabelConstraints, 3) - require.Equal(t, pd.LabelConstraint{ - Op: pd.NotIn, - Key: EngineLabelKey, - Values: []string{EngineLabelTiFlash}, - }, bundle.Rules[0].LabelConstraints[2]) + require.Len(t, bundle.Rules[0].LabelConstraints, 2) // merge rules3, err := newRules(pd.Follower, 4, "") @@ -986,13 +982,7 @@ func TestTidy(t *testing.T) { require.Equal(t, "0", bundle.Rules[0].ID) require.Equal(t, "1", bundle.Rules[1].ID) require.Equal(t, 9, bundle.Rules[1].Count) - require.Equal(t, []pd.LabelConstraint{ - { - Op: pd.NotIn, - Key: EngineLabelKey, - Values: []string{EngineLabelTiFlash}, - }, - }, bundle.Rules[1].LabelConstraints) + require.Equal(t, 0, len(bundle.Rules[1].LabelConstraints)) require.Equal(t, []string{"zone", "host"}, bundle.Rules[1].LocationLabels) } err = bundle.Tidy() @@ -1009,13 +999,6 @@ func TestTidy(t *testing.T) { err = bundle2.Tidy() require.NoError(t, err) require.Equal(t, bundle, bundle2) - - bundle.Rules[1].LabelConstraints = append(bundle.Rules[1].LabelConstraints, pd.LabelConstraint{ - Op: pd.In, - Key: EngineLabelKey, - Values: []string{EngineLabelTiFlash}, - }) - require.ErrorIs(t, bundle.Tidy(), ErrConflictingConstraints) } func TestTidy2(t *testing.T) { @@ -1468,12 +1451,6 @@ func TestTidy2(t *testing.T) { for i, rule := range tt.bundle.Rules { expectedRule := tt.expected.Rules[i] - // Tiflash is always excluded from the constraints. - AddConstraint(&expectedRule.LabelConstraints, pd.LabelConstraint{ - Op: pd.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/pkg/ddl/placement/constraint.go b/pkg/ddl/placement/constraint.go index a7463cd897f56..8d017c93523c3 100644 --- a/pkg/ddl/placement/constraint.go +++ b/pkg/ddl/placement/constraint.go @@ -54,6 +54,7 @@ func NewConstraint(label string) (pd.LabelConstraint, error) { return r, fmt.Errorf("%w: %s", ErrInvalidConstraintFormat, label) } + // Does not allow adding rule of tiflash. if op == pd.In && key == EngineLabelKey && strings.ToLower(val) == EngineLabelTiFlash { return r, fmt.Errorf("%w: %s", ErrUnsupportedConstraint, label) } diff --git a/pkg/ddl/placement/constraint_test.go b/pkg/ddl/placement/constraint_test.go index 577cbf0d0f837..8099e785e7c4f 100644 --- a/pkg/ddl/placement/constraint_test.go +++ b/pkg/ddl/placement/constraint_test.go @@ -64,6 +64,15 @@ func TestNewConstraint(t *testing.T) { Values: []string{"tiflash"}, }, }, + { + name: "not tiflash_compute", + input: "-engine = tiflash_compute ", + label: pd.LabelConstraint{ + Key: "engine", + Op: pd.NotIn, + Values: []string{"tiflash_compute"}, + }, + }, { name: "disallow tiflash", input: "+engine=Tiflash", From 7ae26a64d0010e7435ba68a905fa786b1afb833a Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Tue, 31 Dec 2024 18:46:23 +0800 Subject: [PATCH 2/2] Refine comments Signed-off-by: JaySon-Huang --- pkg/ddl/placement/bundle.go | 4 ++-- pkg/ddl/placement/bundle_test.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/ddl/placement/bundle.go b/pkg/ddl/placement/bundle.go index 0dbb985af86b0..4771c3a541d11 100644 --- a/pkg/ddl/placement/bundle.go +++ b/pkg/ddl/placement/bundle.go @@ -314,8 +314,8 @@ func (b *Bundle) String() string { // Tidy will post optimize Rules, trying to generate rules that suits PD. func (b *Bundle) Tidy() error { - // refer to #58633 - // Note that does not explicitly set rule with label.key==EngineLabelKey, because the + // refer to tidb#58633 + // Does not explicitly set exclude rule with label.key==EngineLabelKey, because the // PD may wrongly add peer to the unexpected stores if that key is specified. tempRules := b.Rules[:0] id := 0 diff --git a/pkg/ddl/placement/bundle_test.go b/pkg/ddl/placement/bundle_test.go index 2991ed996ee37..a280bc15d8896 100644 --- a/pkg/ddl/placement/bundle_test.go +++ b/pkg/ddl/placement/bundle_test.go @@ -348,6 +348,7 @@ func TestString(t *testing.T) { rules2, err := newRules(pd.Voter, 4, `["-zone=sh", "+zone=bj"]`) require.NoError(t, err) rules3, err := newRules(pd.Voter, 3, `["-engine=tiflash", "-engine=tiflash_compute"]`) + require.NoError(t, err) bundle.Rules = append(append(rules1, rules2...), rules3...) require.Equal(t, "{\"group_id\":\"TiDB_DDL_1\",\"group_index\":0,\"group_override\":false,\"rules\":[{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":3,\"label_constraints\":[{\"key\":\"zone\",\"op\":\"in\",\"values\":[\"sh\"]}]},{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":4,\"label_constraints\":[{\"key\":\"zone\",\"op\":\"notIn\",\"values\":[\"sh\"]},{\"key\":\"zone\",\"op\":\"in\",\"values\":[\"bj\"]}]},{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":3,\"label_constraints\":[{\"key\":\"engine\",\"op\":\"notIn\",\"values\":[\"tiflash\"]},{\"key\":\"engine\",\"op\":\"notIn\",\"values\":[\"tiflash_compute\"]}]}]}", bundle.String())