ddl: implement the placement rules inheritance logic #28365

AilinKid · 2021-09-26T08:35:52Z

Signed-off-by: ailinkid 314806019@qq.com

What problem does this PR solve?

Issue Number: close #28364

What is changed and how it works?

How it Works:
avoid to write the default placement bundle to pd again when creating table

Check List

Tests

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

tiup playground nightly --tiflash 0  --db 2 --db.binpath  ~/go/src/github.com/pingcap/tidb/bin/tidb-server
http://127.0.0.1:2379/pd/api/v1/config/placement-rule  // check the placement must only one ------ PD default rules
create table t(a int) constraints="[-region=ssd]";           // create a test table with direct policy
http://127.0.0.1:2379/pd/api/v1/config/placement-rule  // check the placement rules again  ------ PD default + Table specified
check the rule for `index` and `override` field, it must be the `IndexTable` and `True`

Inaddition:
placement-rules-inheritance-test

Release note

None

ti-chi-bot · 2021-09-26T08:35:53Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lcwangchao
xhebox

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

I think it makes sense that we have a function like Bundle.SetType(db | tbl). But that can be done later.

ddl/table.go

AilinKid · 2021-09-28T03:46:31Z

/run-check_dev

ddl/table.go

xhebox

Please:

Refactor Reset(index, ids...int64), remove/integrate Append and xxxBundleWrapper within it. For simplicity, better keep only one reset function.
Remove all bundle.Tidy(), move them into infosync.PutRuleBundles.
Remove checkPolicyValidation here, it is duplicated because of NewBundleFromOptions.

The code will be much simpler with above changes.

ddl/placement/bundle.go

ddl/table.go

xhebox · 2021-09-28T05:40:27Z

ddl/table.go

+	err = bundle.Tidy()
+	if err != nil {
+		return nil, errors.Trace(err)
+	}


Reset first, then Tidy. Maybe we could move all Tidy to infosync.PutRuleBundles.

Tidy first is reasonable. Since tidy wanna append a NotIn TiFlash rule, it should be done before reset work.
The later will clone all these rule for other new ids. otherwise, we should append NotIn TiFlash rule for every independent id inferred from startKey/ EndKey

Tidy was intended to be used as an post-optimization. Maybe we should move the append of -engine=tiflash into reset.

I put it into the new bundle portal function --- NewBundleFromOptions, which are much more clear.

ddl/table.go

AilinKid · 2021-09-30T03:48:59Z

placement-rules-inheritance-test.md

here is the manual test script and test results

lcwangchao · 2021-10-01T08:20:22Z

ddl/placement/bundle.go

-		rule.GroupID = b.ID
-		rule.StartKeyHex = startKey
-		rule.EndKeyHex = endKey
+func (b *Bundle) Reset(ruleIndex int, newIDs []int64) *Bundle {


The current code about bundles seems some what weird and a little complex now. How about refactoring them to below methods:

// Build bundle from table info, and the partition rules and included in the return NewBundleFromTable(tbl *model.TableInfo) (*Bundle, error) // Build bundle from partition NewBundleFromPartition(partition *model.PartitionDefinition) (*Bundle, error)

I think the above methods is enough for ddl to use. It's not necessary to use BundleByName in infoschema because we'll finally delete it and the logic that directly building bundle from meta is more brief and easy to maintain.

We do not need method Tidy too. We can tidy the bundle when we constructing it, I think no where requires a not tidied bundle.

Another advices is the reuse of the code, for example, we can introduce a private method like

newBundleFromPolicyOrDirectOptions(infoschema.InfoSchema, *model.PolicyRefInfo, *model.PlacementSettings) (*Bundle, error)

It will return a semi-finished Bundle and just for code reuse and it should only be visible in placement package

1: refactored to func NewBundleFromTblInfo(t *meta.Meta, job *model.Job, tbInfo *model.TableInfo) and func NewBundleFromPartition(t *meta.Meta, job *model.Job, partition *model.PartitionInfo)
2: tidy moved to constructors--- NewBundleFromOptions
3: the newBundleFromPolicyOrDirectOptions function is built

@AilinKid, where is the change of 2? I don't see that.

added, missed in my commits

Signed-off-by: ailinkid <314806019@qq.com>

AilinKid · 2021-10-08T08:41:20Z

/run-check_dev_2

xhebox · 2021-10-08T09:18:29Z

/merge

ti-chi-bot · 2021-10-08T09:18:32Z

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

Commit hash: 6806823

AilinKid force-pushed the avoid-sync-default-bundle-to-pd branch from a4b412d to 914db2d Compare September 26, 2021 11:10

xhebox approved these changes Sep 27, 2021

View reviewed changes

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

xhebox requested a review from morgo September 27, 2021 01:13

xhebox mentioned this pull request Sep 27, 2021

Many useless rules and group-bundle is not override #28353

Closed

lcwangchao reviewed Sep 27, 2021

View reviewed changes

ddl/table.go Show resolved Hide resolved

ddl/table.go Outdated Show resolved Hide resolved

ddl/table.go Outdated Show resolved Hide resolved

ddl/table.go Outdated Show resolved Hide resolved

ddl/table.go Outdated Show resolved Hide resolved

AilinKid removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 28, 2021

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/table.go Outdated Show resolved Hide resolved

ddl/table.go Outdated Show resolved Hide resolved

xhebox requested changes Sep 28, 2021

View reviewed changes

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed status/LGT1 Indicates that a PR has LGTM 1. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 28, 2021

AilinKid changed the title ~~ddl: use the persistent policy in parent objects and avoid use pd default bundle~~ ddl: implement the placement rules inheritance logic Sep 29, 2021

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 30, 2021

lcwangchao reviewed Oct 1, 2021

View reviewed changes

AilinKid force-pushed the avoid-sync-default-bundle-to-pd branch from 7eec66d to 3c72015 Compare October 2, 2021 00:58

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 2, 2021

morgo requested a review from xhebox October 6, 2021 19:28

lcwangchao approved these changes Oct 8, 2021

View reviewed changes

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

AilinKid added 10 commits October 8, 2021 14:51

show the database and partition rules

edf50df

Signed-off-by: ailinkid <314806019@qq.com>

fix comment blank

f08f0ff

Signed-off-by: ailinkid <314806019@qq.com>

fix the placement sql test

d94d838

Signed-off-by: ailinkid <314806019@qq.com>

fix the rules id

905107b

Signed-off-by: ailinkid <314806019@qq.com>

address comment from chao and he

6e73fa4

Signed-off-by: ailinkid <314806019@qq.com>

.

f6cd6a9

Signed-off-by: ailinkid <314806019@qq.com>

fix test

23c81fc

Signed-off-by: ailinkid <314806019@qq.com>

fix bundle

4545f19

Signed-off-by: ailinkid <314806019@qq.com>

fix test

7050f69

Signed-off-by: ailinkid <314806019@qq.com>

change the rule de-duplication to index indentification

247a8df

Signed-off-by: ailinkid <314806019@qq.com>

AilinKid force-pushed the avoid-sync-default-bundle-to-pd branch from 08d26e7 to 247a8df Compare October 8, 2021 06:51

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

move tidy to constructors: NewBundleFromOptions

ad907f3

Signed-off-by: ailinkid <314806019@qq.com>

AilinKid force-pushed the avoid-sync-default-bundle-to-pd branch from 173a4d0 to ad907f3 Compare October 8, 2021 07:02

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 8, 2021

AilinKid added 5 commits October 8, 2021 15:04

fmt

e606439

Signed-off-by: ailinkid <314806019@qq.com>

Merge branch 'master' into avoid-sync-default-bundle-to-pd

45a2081

Merge branch 'master' into avoid-sync-default-bundle-to-pd

1ae67cb

fix test

dcb801e

Signed-off-by: ailinkid <314806019@qq.com>

Merge branch 'master' into avoid-sync-default-bundle-to-pd

6806823

xhebox approved these changes Oct 8, 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 8, 2021

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

ti-chi-bot merged commit b63bc2b into pingcap:master Oct 8, 2021

AilinKid mentioned this pull request Oct 21, 2021

Define the placement of data by SQL statements #18030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: implement the placement rules inheritance logic #28365

ddl: implement the placement rules inheritance logic #28365

AilinKid commented Sep 26, 2021 •

edited

Loading

ti-chi-bot commented Sep 26, 2021 •

edited

Loading

xhebox left a comment

AilinKid commented Sep 28, 2021

xhebox left a comment •

edited

Loading

xhebox Sep 28, 2021

AilinKid Sep 28, 2021

xhebox Sep 29, 2021

AilinKid Oct 2, 2021

AilinKid commented Sep 30, 2021 •

edited

Loading

lcwangchao Oct 1, 2021 •

edited

Loading

AilinKid Oct 2, 2021

xhebox Oct 8, 2021

AilinKid Oct 8, 2021

AilinKid commented Oct 8, 2021

xhebox commented Oct 8, 2021

ti-chi-bot commented Oct 8, 2021

ddl: implement the placement rules inheritance logic #28365

ddl: implement the placement rules inheritance logic #28365

Conversation

AilinKid commented Sep 26, 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 26, 2021 • edited Loading

xhebox left a comment

Choose a reason for hiding this comment

AilinKid commented Sep 28, 2021

xhebox left a comment • edited Loading

Choose a reason for hiding this comment

xhebox Sep 28, 2021

Choose a reason for hiding this comment

AilinKid Sep 28, 2021

Choose a reason for hiding this comment

xhebox Sep 29, 2021

Choose a reason for hiding this comment

AilinKid Oct 2, 2021

Choose a reason for hiding this comment

AilinKid commented Sep 30, 2021 • edited Loading

lcwangchao Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

AilinKid Oct 2, 2021

Choose a reason for hiding this comment

xhebox Oct 8, 2021

Choose a reason for hiding this comment

AilinKid Oct 8, 2021

Choose a reason for hiding this comment

AilinKid commented Oct 8, 2021

xhebox commented Oct 8, 2021

ti-chi-bot commented Oct 8, 2021

AilinKid commented Sep 26, 2021 •

edited

Loading

ti-chi-bot commented Sep 26, 2021 •

edited

Loading

xhebox left a comment •

edited

Loading

AilinKid commented Sep 30, 2021 •

edited

Loading

lcwangchao Oct 1, 2021 •

edited

Loading