placement: refine the bundle contruction logic #28450

xhebox · 2021-09-28T04:47:34Z

What problem does this PR solve?

Issue Number: #18030

Problem Summary: Several days ago, we had a meeting and decided to change the logic a little bit, refer weekly report.

What is changed and how it works?

voterConstraints and voters are removed. Later parser should remove the counter part, too.
NewMergeRules is replaced with NewBundleFromOptions for validation.
primaryRegion must be included in regions.
Add locationLabels: [region, zone, rack, host] to every rule.
Use locationLabels to balance the schedule automatically, rather than constructing for every region separately. It is safe since PD will exhaust all combinations recursively for the best isolation result.
Introduce NewConstraintsDirect and NewConstraintDirect helper.

Check List

Tests

Unit test
Integration test

Documentation

Contains syntax changes docs: update design doc for voter_constraints #28021

Release note

None

ti-chi-bot · 2021-09-28T04:47:35Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

AilinKid
lcwangchao

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

xhebox · 2021-09-28T04:49:58Z

ddl/placement/bundle.go

-	if uint64(len(regions)) > followers {
-		return nil, fmt.Errorf("%w: remain %d region to schedule, only %d follower left", ErrInvalidPlacementOptions, uint64(len(regions)), followers)
-	}
-
-	if len(regions) == 0 {
-		constraints, err := NewConstraints(nil)
-		if err != nil {
-			return nil, err
-		}
-		Rules = append(Rules, NewRule(Follower, followers, constraints))
-	} else {
-		count := followers / uint64(len(regions))
-		rem := followers - count*uint64(len(regions))
-		for _, region := range regions {
-			constraints, err = NewConstraints([]string{fmt.Sprintf("+region=%s", region)})
-			if err != nil {
-				return nil, fmt.Errorf("%w: invalid region of 'Regions', '%s'", err, region)
-			}
-			replica := count
-			if rem > 0 {
-				replica += 1
-				rem--
-			}
-			Rules = append(Rules, NewRule(Follower, replica, constraints))
-		}
-	}
-


The old logic is replaced by region in [a,b,c] completely, which relies on PD to balance(the result is deterministic). And to balance it perfectly, we must set location labels manually for every rule. PD will score every schedule combination to see if every single rule has the best possible isolation, i.e. peers are far from each other.

Previously we will construct region in [a] for every region a.

To make PD balance the peers, I think it is required that the label must be included in the LocationLabels?

Signed-off-by: xhe <xw897002528@gmail.com>

lcwangchao · 2021-09-28T06:53:22Z

ddl/placement/bundle.go

-	if uint64(len(regions)) > followers {
-		return nil, fmt.Errorf("%w: remain %d region to schedule, only %d follower left", ErrInvalidPlacementOptions, uint64(len(regions)), followers)
-	}
-
-	if len(regions) == 0 {
-		constraints, err := NewConstraints(nil)
-		if err != nil {
-			return nil, err
-		}
-		Rules = append(Rules, NewRule(Follower, followers, constraints))
-	} else {
-		count := followers / uint64(len(regions))
-		rem := followers - count*uint64(len(regions))
-		for _, region := range regions {
-			constraints, err = NewConstraints([]string{fmt.Sprintf("+region=%s", region)})
-			if err != nil {
-				return nil, fmt.Errorf("%w: invalid region of 'Regions', '%s'", err, region)
-			}
-			replica := count
-			if rem > 0 {
-				replica += 1
-				rem--
-			}
-			Rules = append(Rules, NewRule(Follower, replica, constraints))
-		}
-	}
-


To make PD balance the peers, I think it is required that the label must be included in the LocationLabels?

ddl/placement/bundle.go

xhebox · 2021-09-28T07:24:01Z

To make PD balance the peers, I think it is required that the label must be included in the LocationLabels?

Here:

https://github.com/pingcap/tidb/pull/28450/files#diff-0b441afe10e04791ae9ef8a5c351c9e75c6477867ae8db765d70df1ded3da6dbR56-R62

For sugar options, there are only 4 location labels, according to https://github.com/pingcap/tidb/blob/master/docs/design/2020-06-24-placement-rules-in-sql.md#direct-assignment.

Signed-off-by: xhe <xw897002528@gmail.com>

ddl/placement/bundle.go

Signed-off-by: xhe <xw897002528@gmail.com>

ddl/placement/bundle.go

Signed-off-by: xhe <xw897002528@gmail.com>

AilinKid

Rest LGTM

AilinKid · 2021-09-30T09:07:05Z

ddl/placement/rule.go

+		Role:           role,
+		Count:          int(replicas),
+		Constraints:    cnst,
+		LocationLabels: []string{"region", "zone", "rack", "host"},


just one question: what this two fields is for?

For isolation scores, it is used to compare which schedule(in all possible schedule combination cases) is better. Say region=sh,bj,us, 2,1,0 will have a lower score than 1,1,1 because of the existence of LocationLaels: []string{..., region, ...}. Similar arguments apply to zone,rack,host.

As for IsolationLevel, it means two locations are far enough and will affect schedule behaviors. Refer https://github.com/tikv/pd/blob/740fac18e89fa1754a40f5ec885b5e5b6caa2471/server/schedule/filter/filters.go#L696-L701.

Signed-off-by: xhe <xw897002528@gmail.com>

AilinKid · 2021-10-02T00:26:30Z

/merge

ti-chi-bot · 2021-10-02T00:26:33Z

This pull request has been accepted and is ready to merge.

Commit hash: 946e6ab

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 28, 2021

xhebox requested review from AilinKid and lcwangchao September 28, 2021 04:47

xhebox commented Sep 28, 2021

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2021

xhebox added 7 commits September 28, 2021 13:28

placement: remove voters

17150b1

Signed-off-by: xhe <xw897002528@gmail.com>

placement: silence about number of followers

bb9b203

Signed-off-by: xhe <xw897002528@gmail.com>

placement: 3 voter default rules for empty settings

2ae20e6

Signed-off-by: xhe <xw897002528@gmail.com>

placement: new schedule logic for sugar

4b820b3

Signed-off-by: xhe <xw897002528@gmail.com>

placement: fix tests for the previous commit

54ab81b

Signed-off-by: xhe <xw897002528@gmail.com>

placement: replacement mergeRule check with bundle construction

f719a2f

Signed-off-by: xhe <xw897002528@gmail.com>

placement: fix tests

2d5905e

Signed-off-by: xhe <xw897002528@gmail.com>

xhebox force-pushed the placement_4 branch from fb143da to 2d5905e Compare September 28, 2021 05:29

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2021

xhebox mentioned this pull request Sep 28, 2021

Define the placement of data by SQL statements #18030

Closed

xhebox added 2 commits September 28, 2021 14:12

ddl: dont need so much tests, it is tested by unit tests

de7d597

Signed-off-by: xhe <xw897002528@gmail.com>

executor: duplicated tests

5a815d6

Signed-off-by: xhe <xw897002528@gmail.com>

lcwangchao reviewed Sep 28, 2021

View reviewed changes

ddl: fix tests

2a417a6

Signed-off-by: xhe <xw897002528@gmail.com>

lcwangchao approved these changes Sep 28, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 28, 2021

lcwangchao reviewed Sep 28, 2021

View reviewed changes

ddl/placement/bundle.go Outdated Show resolved Hide resolved

xhebox added 2 commits September 28, 2021 22:01

placement: address comments

46a5c24

Signed-off-by: xhe <xw897002528@gmail.com>

priv: fix tests

0917f58

Signed-off-by: xhe <xw897002528@gmail.com>

lcwangchao reviewed Sep 28, 2021

View reviewed changes

ddl/placement/bundle.go Outdated Show resolved Hide resolved

morgo and others added 2 commits September 28, 2021 16:56

Merge branch 'master' into placement_4

e1ef2a9

placement: get the ceil function back

89a96b0

Signed-off-by: xhe <xw897002528@gmail.com>

AilinKid reviewed Sep 30, 2021

View reviewed changes

AilinKid and others added 3 commits September 30, 2021 17:07

Merge branch 'master' into placement_4

5cc3163

placement: don't add new rule if count is zero, fix tests

244f379

Signed-off-by: xhe <xw897002528@gmail.com>

Merge branch 'master' into placement_4

946e6ab

AilinKid approved these changes Oct 2, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 2, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 2, 2021

ti-chi-bot merged commit 2a02e6e into pingcap:master Oct 2, 2021

xhebox deleted the placement_4 branch October 29, 2021 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

placement: refine the bundle contruction logic #28450

placement: refine the bundle contruction logic #28450

xhebox commented Sep 28, 2021 •

edited

Loading

ti-chi-bot commented Sep 28, 2021 •

edited

Loading

xhebox Sep 28, 2021 •

edited

Loading

lcwangchao Sep 28, 2021

lcwangchao Sep 28, 2021

xhebox commented Sep 28, 2021

AilinKid left a comment

AilinKid Sep 30, 2021

xhebox Sep 30, 2021 •

edited

Loading

AilinKid Oct 2, 2021

AilinKid commented Oct 2, 2021

ti-chi-bot commented Oct 2, 2021

placement: refine the bundle contruction logic #28450

placement: refine the bundle contruction logic #28450

Conversation

xhebox commented Sep 28, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 28, 2021 • edited Loading

xhebox Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

lcwangchao Sep 28, 2021

Choose a reason for hiding this comment

lcwangchao Sep 28, 2021

Choose a reason for hiding this comment

xhebox commented Sep 28, 2021

AilinKid left a comment

Choose a reason for hiding this comment

AilinKid Sep 30, 2021

Choose a reason for hiding this comment

xhebox Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

AilinKid Oct 2, 2021

Choose a reason for hiding this comment

AilinKid commented Oct 2, 2021

ti-chi-bot commented Oct 2, 2021

xhebox commented Sep 28, 2021 •

edited

Loading

ti-chi-bot commented Sep 28, 2021 •

edited

Loading

xhebox Sep 28, 2021 •

edited

Loading

xhebox Sep 30, 2021 •

edited

Loading