placement: merge the rules if the constraints are the same

[nolouch]

What problem does this PR solve?

Issue Number: close #45395

Problem Summary:

 create placement policy retail_pos constraints="[+retail_region=fast]";
 CREATE TABLE t1 (a INT);
 ALTER TABLE t1  PLACEMENT POLICY=retail_pos;

The rules in PD side:

[
  {
    "group_id": "TiDB_DDL_90",
    "id": "table_rule_90_0",
    "index": 40,
    "start_key": "7480000000000000ff5a00000000000000f8",
    "end_key": "7480000000000000ff5b00000000000000f8",
    "role": "leader",
    "is_witness": false,
    "count": 1,
    "label_constraints": [
      {
        "key": "retail_region",
        "op": "in",
        "values": [
          "fast"
        ]
      },
      ....
    ],
    "create_timestamp": 1689589620
  },
  {
    "group_id": "TiDB_DDL_90",
    "id": "table_rule_90_1",
    "index": 40,
    "start_key": "7480000000000000ff5a00000000000000f8",
    "end_key": "7480000000000000ff5b00000000000000f8",
    "role": "voter",
    "is_witness": false,
    "count": 2,
    "label_constraints": [
      {
        "key": "retail_region",
        "op": "in",
        "values": [
          "fast"
        ]
      },
      ...
    ],
    "create_timestamp": 1689589620
  },
  {
    "group_id": "pd",
    "id": "default",
    "start_key": "",
    "end_key": "",
    "role": "voter",
    "is_witness": false,
    "count": 3
  }
]

There is no need to split into two rules because we do not need to distinguish roles, two rules perhaps exist some problems. such as location-labels may not work in tikv/pd#6637

What is changed and how it works?

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No code

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

[codecov]

Codecov Report

Merging #46529 (2d9de72) into master (d3d30f5) will decrease coverage by 0.5134%.
Report is 178 commits behind head on master.
The diff coverage is 92.1739%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46529        +/-   ##
================================================
- Coverage   73.3326%   72.8193%   -0.5134%     
================================================
  Files          1322       1361        +39     
  Lines        396474     407518     +11044     
================================================
+ Hits         290745     296752      +6007     
- Misses        87208      92033      +4825     
- Partials      18521      18733       +212     

Flag	Coverage Δ
integration	33.4624% <83.4782%> (?)
unit	73.0971% <92.1739%> (-0.2356%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	53.9913% <ø> (-0.0532%)	⬇️
parser	84.9484% <ø> (-0.0234%)	⬇️
br	48.8855% <ø> (-3.7563%)	⬇️

[nolouch]

/retest-required

[nolouch]

/retest-required

[nolouch]

/retest-required

[hawkingrei]

slices.SortStableFunction is better. it can remove BCE.

[nolouch]

PTAL @CabinfeverB @JmPotato

[nolouch]

/test unit-test

[tiprow]

@nolouch: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test unit-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lcwangchao]

Could you add some comments here or somewhere to explain why when two or more groups have a leader or voter role, we should skip MergeTransformableRoles?

[CabinfeverB]

I agree. But if 1 leader and 2 followers in group1 and 2 voters in group2, can we merge group1 to voters?

[xhebox]

I agree. But if 1 leader and 2 followers in group1 and 2 voters in group2, can we merge group1 to voters?

No. They don't share same constraints.

[nolouch]

I agree. But if 1 leader and 2 followers in group1 and 2 voters in group2, can we merge group1 to voters?

that means the group 1 has leader priority than group 2, so it's not transformable. I added comments.

[lcwangchao]

Could you add some extra checks for rules in Tidy? I think Tidy can only be called when some fields are empty (StartKeyHex/EndKeyHex .etc).

[CabinfeverB]

Could you add some extra checks for rules in Tidy? I think Tidy can only be called when some fields are empty (StartKeyHex/EndKeyHex .etc).

Does it mean Bundle is not associated with a table?

[nolouch]

As the comments saied, it's optimized rules, try to simplify the rules.

[CabinfeverB]

Could you add some extra checks for rules in Tidy? I think Tidy can only be called when some fields are empty (StartKeyHex/EndKeyHex .etc).

Does it mean Bundle is not associated with a table?

[CabinfeverB]

The maximum length is 2?

[nolouch]

maybe more.

[CabinfeverB]

I agree. But if 1 leader and 2 followers in group1 and 2 voters in group2, can we merge group1 to voters?

[CabinfeverB]

And I have a question: as PR description, the cause of multiple rules is multiple tables instead of role?

[nolouch]

And I have a question: as PR description, the cause of multiple rules is multiple tables instead of role?

it is caused by multi-role setting. see

tidb/ddl/placement/bundle.go

Lines 57 to 135 in 2472154

	func NewBundleFromConstraintsOptions(options *model.PlacementSettings) (*Bundle, error) {
	if options == nil {
	return nil, fmt.Errorf("%w: options can not be nil", ErrInvalidPlacementOptions)
	}
	if len(options.PrimaryRegion) > 0 || len(options.Regions) > 0 || len(options.Schedule) > 0 {
	return nil, fmt.Errorf("%w: should be [LEADER/VOTER/LEARNER/FOLLOWER]_CONSTRAINTS=.. [VOTERS/FOLLOWERS/LEARNERS]=.., mixed other sugar options %s", ErrInvalidPlacementOptions, options)
	}
	constraints := options.Constraints
	leaderConst := options.LeaderConstraints
	learnerConstraints := options.LearnerConstraints
	followerConstraints := options.FollowerConstraints
	followerCount := options.Followers
	learnerCount := options.Learners
	commonConstraints, err := NewConstraintsFromYaml([]byte(constraints))
	if err != nil {
	return nil, fmt.Errorf("%w: 'Constraints' should be [constraint1, ...] or any yaml compatible array representation", err)
	}
	rules := []*Rule{}
	leaderConstraints, err := NewConstraintsFromYaml([]byte(leaderConst))
	if err != nil {
	return nil, fmt.Errorf("%w: 'LeaderConstraints' should be [constraint1, ...] or any yaml compatible array representation", err)
	}
	for _, cnst := range commonConstraints {
	if err := leaderConstraints.Add(cnst); err != nil {
	return nil, fmt.Errorf("%w: LeaderConstraints conflicts with Constraints", err)
	}
	}
	rules = append(rules, NewRule(Leader, 1, leaderConstraints))
	followerRules, err := NewRules(Voter, followerCount, followerConstraints)
	if err != nil {
	return nil, fmt.Errorf("%w: invalid FollowerConstraints", err)
	}
	for _, rule := range followerRules {
	// give a default of 2 followers
	if rule.Count == 0 {
	rule.Count = 2
	}
	for _, cnst := range commonConstraints {
	if err := rule.Constraints.Add(cnst); err != nil {
	return nil, fmt.Errorf("%w: FollowerConstraints conflicts with Constraints", err)
	}
	}
	}
	rules = append(rules, followerRules...)
	learnerRules, err := NewRules(Learner, learnerCount, learnerConstraints)
	if err != nil {
	return nil, fmt.Errorf("%w: invalid LearnerConstraints", err)
	}
	for _, rule := range learnerRules {
	if rule.Count == 0 {
	if len(rule.Constraints) > 0 {
	return nil, fmt.Errorf("%w: specify learner constraints without specify how many learners to be placed", ErrInvalidPlacementOptions)
	}
	}
	for _, cnst := range commonConstraints {
	if err := rule.Constraints.Add(cnst); err != nil {
	return nil, fmt.Errorf("%w: LearnerConstraints conflicts with Constraints", err)
	}
	}
	if rule.Count > 0 {
	rules = append(rules, rule)
	}
	}
	labels, err := newLocationLabelsFromSurvivalPreferences(options.SurvivalPreferences)
	if err != nil {
	return nil, err
	}
	for _, rule := range rules {
	rule.LocationLabels = labels
	}
	return &Bundle{Rules: rules}, nil
	}

[nolouch]

PTAL @lcwangchao @xhebox @CabinfeverB thanks.

[CabinfeverB]

/test unit-test

[tiprow]

@CabinfeverB: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test unit-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CabinfeverB, xhebox

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [xhebox]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2023-09-22 08:47:39.758424669 +0000 UTC m=+5449.476766885: ☑️ agreed by xhebox.
• 2023-09-25 08:25:32.05808706 +0000 UTC m=+263321.776429276: ☑️ agreed by CabinfeverB.

[nolouch]

/test unit-test

[tiprow]

@nolouch: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test unit-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.