ddl: Fixed partitioning a non-partitioned table with placement rules #57560

mjonss · 2024-11-20T10:42:34Z

What problem does this PR solve?

Issue Number: close #55705

Problem Summary:
When updating the placement rule bundle, the table ID was included both as the table and the partition id.

What changed and how does it work?

Not adding it as partition id for non-partitioned table (which uses the original table as a single partition, hence the same ID).

Also simplified the bundle handling during schema update / ApplyDiff, by removing logic that:

first went through all partitions and marked the partition to update the bundle, then went through all partition updates and updated its table

to

mark the table directly that it should update the bundle.

For REORGANIZE PARTITION, there the bundles will be updated twice, first with the intermediate set of partitions (both old and new), so that data copying and index creations on the new partitions will follow the correct policies directly, and then with the final partition set, avoiding having to move the data again when the DDL is done.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

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

fixed an issue with partitioning a non-partitioned table with placement rules.

tiprow · 2024-11-20T10:42:50Z

Hi @mjonss. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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-sigs/prow repository.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no suggestions.

lcwangchao · 2024-11-20T10:58:11Z

pkg/domain/infosync/placement_manager.go

+		end   string
+	}
+	keys := make([]keyRange, 0, rules)
+	for k := range m.bundles {


I think we only need to check the rules inside one bundle just now. If GroupBundle.Override is set to true, it is allowed to have multiple leader rules.

And in one RuleGroup when a rule has Override==true, it should also be skipped.

So the below is OK?

if rule.Role == pd.Leader && !m.bundles[k].Override { keys = append(keys, keyRange{start: k + ":" + rule.StartKeyHex, end: k + ":" + rule.EndKeyHex}) }

We still need to check the rules in one bundle even bundle[k].Override is true, but the current implementation skips all rules in such bundle?

We still need to check the rules in one bundle even bundle[k].Override is true

true.

@lcwangchao @xhebox please review the new duplicate leader range check.

So, why not only validating the input bundles one by one? I think the current version does not care the overlaps across different rules.

pkg/ddl/placement_policy_test.go

codecov · 2024-11-20T11:15:05Z

Codecov Report

Attention: Patch coverage is 83.33333% with 17 lines in your changes missing coverage. Please review.

Project coverage is 74.8011%. Comparing base (ad5ca42) to head (2f97ac7).
Report is 31 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57560        +/-   ##
================================================
+ Coverage   72.8329%   74.8011%   +1.9681%     
================================================
  Files          1676       1721        +45     
  Lines        463539     478136     +14597     
================================================
+ Hits         337609     357651     +20042     
+ Misses       105125      98140      -6985     
- Partials      20805      22345      +1540

Flag	Coverage Δ
integration	`49.2534% <66.6666%> (?)`
unit	`72.2204% <83.3333%> (+0.0011%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`60.9294% <ø> (+15.5728%)`	⬆️

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

pkg/ddl/partition.go

pkg/ddl/placement_policy_test.go

ti-chi-bot · 2024-11-20T14:36:48Z

[LGTM Timeline notifier]

Timeline:

2024-11-20 14:36:48.070107027 +0000 UTC m=+42395.689761543: ☑️ agreed by Defined2014.

…bundles anyway

…the final one.

pkg/infoschema/builder.go

ti-chi-bot · 2024-11-21T02:50:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Defined2014
Once this PR has been reviewed and has the lgtm label, please assign gmhdbjd for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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 · 2024-11-21T02:50:40Z

@Defined2014: Your lgtm message is repeated, so it is ignored.

In response to this:

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-sigs/prow repository.

lcwangchao · 2024-11-21T06:30:17Z

pkg/ddl/partition.go

+	// - not removing any placement rules for removed partitions
+	// - not leaving anything in case of failure/rollback!
+	// So we will:
+	// 1) First write the new bundles including both new and old partitions,


Why the new bundles should include old partitions? The old bundles have already defined these rules and still take effects here.

Oh, so if the Bundle is overwritten, the older bundle rules will still be in effect?
Like if the first bundle had rules for table id 111, part id 112 and 113, and the new bundle would have table id 111, partition id 113 and 114, would part id 112 still be placed as the old bundle rules or would it be placed without any rules?

or would it be placed without any rules?

This, I believe.

// it will filter the rules depends on the interval index override in the same group or the // group-index override between different groups // For example, given rules: // ruleA: group_id: 4, id: 2, override: true // ruleB: group_id: 4, id: 1, override: true // ruleC: group_id: 3 // ruleD: group_id: 2 // RuleGroupA: id:4, override: false // RuleGroupB: id:3, override: true // RuleGroupC: id:2, override: false // Finally only ruleA and ruleC will be selected.

A better example:

CREATE TABLE t (a int) partition by range (a) (partition p0 values less than (1000000), partition p1M values less than (2000000)) placement policy pp1; ALTER TABLE t REORGANIZE PARTITION p1M INTO (PARTITION p1M values less than (2000000), partition p2M values less than (3000000));

During the DDL we would still want the original p1M to follow the table level rule, so we keep it in the bundle.
When the final bundles will be created it will not be included.
Since the table keeps its table id (for REORGANIZE PARTITION) there are no other bundles that covers the inherited table's placement rules for the original partition p1M, which means we need to cover it during the time of the DDL, since it can still be accessed (and double written).

Another alternative would be to create partition level bundles for the old replaced partitions, and let them be removed by GC later, but I think that would qualify for a separate PR.

pkg/ddl/placement_policy_test.go

pkg/ddl/partition.go

lcwangchao · 2024-11-21T06:39:42Z

@xhebox PTAL

…cement rules

pkg/ddl/partition.go

mjonss · 2024-11-22T15:56:38Z

/retest

tiprow · 2024-11-22T15:57:00Z

@mjonss: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

lcwangchao · 2024-11-25T03:52:45Z

pkg/domain/infosync/placement_manager.go

+		end   string
+	}
+	keys := make([]keyRange, 0, rules)
+	for k := range m.bundles {


So, why not only validating the input bundles one by one? I think the current version does not care the overlaps across different rules.

lcwangchao · 2024-11-25T03:58:54Z

pkg/domain/infosync/placement_manager.go

+		return keys[i].start < keys[j].start
+	})
+	for i := 1; i < len(keys); i++ {
+		if keys[i].start < keys[i-1].end && !keys[i].override {


If we have two groups, should the below case also report an error if order by group id first?

// when i==1 `keys[i].start < keys[i-1].end` [0] {group_id: 1, override: false, rules: [(start: 0xF0, end: 0xF2)]} [1] {group_id: 2, override: false, rules: [(start: 0x01, end: 0x02)]}

And for below case, I think it should still report an error but seems not

{ group_id: 1, rules: [ {override: false, start: 0x01, end: 0x0F}, {override: true, start: 0x02, end: 0x03}, {override: false, start: 0x04, end: 0x05} ] }

Fixed partitioning a non-partitioned table with placement rules

258638a

mjonss requested review from Copilot and Defined2014 November 20, 2024 10:42

mjonss requested a review from lcwangchao November 20, 2024 10:43

Copilot AI reviewed Nov 20, 2024

View reviewed changes

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Nov 20, 2024

Linting

93a14c9

lcwangchao reviewed Nov 20, 2024

View reviewed changes

pkg/ddl/placement_policy_test.go Show resolved Hide resolved

Defined2014 reviewed Nov 20, 2024

View reviewed changes

pkg/ddl/partition.go Outdated Show resolved Hide resolved

pkg/ddl/placement_policy_test.go Show resolved Hide resolved

Defined2014 approved these changes Nov 20, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 20, 2024

Removed handling of partition bundles, since it was only using table …

9210c20

…bundles anyway

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 20, 2024

mjonss added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2024

mjonss added 2 commits November 21, 2024 01:00

Fixed update placement bundles for partitions

f156b89

In REORG PARTITION we first register the intermediate state and then …

58d859d

…the final one.

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2024

mjonss requested a review from Defined2014 November 21, 2024 00:44

Defined2014 reviewed Nov 21, 2024

View reviewed changes

pkg/infoschema/builder.go Show resolved Hide resolved

Defined2014 approved these changes Nov 21, 2024

View reviewed changes

lcwangchao reviewed Nov 21, 2024

View reviewed changes

pkg/ddl/placement_policy_test.go Outdated Show resolved Hide resolved

lcwangchao reviewed Nov 21, 2024

View reviewed changes

pkg/ddl/placement_policy_test.go Show resolved Hide resolved

lcwangchao reviewed Nov 21, 2024

View reviewed changes

pkg/ddl/partition.go Show resolved Hide resolved

lcwangchao requested a review from xhebox November 21, 2024 06:39

Fixed failure and added tests for rollback of reorg partition and pla…

227ccd8

…cement rules

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 21, 2024

Added more tests and one minor fix

9200fbb

ti-chi-bot 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 Nov 22, 2024

Linting

d58c8d2

lcwangchao reviewed Nov 22, 2024

View reviewed changes

pkg/ddl/partition.go Show resolved Hide resolved

pkg/ddl/partition.go Outdated Show resolved Hide resolved

pkg/ddl/partition.go Outdated Show resolved Hide resolved

mjonss added 2 commits November 22, 2024 13:01

More advanced comparison for bundle rules leader overlap

54abd6d

Renamed a function and added more comments

2f97ac7

lcwangchao reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: Fixed partitioning a non-partitioned table with placement rules #57560

ddl: Fixed partitioning a non-partitioned table with placement rules #57560

mjonss commented Nov 20, 2024 •

edited

Loading

tiprow bot commented Nov 20, 2024

lcwangchao Nov 20, 2024

mjonss Nov 21, 2024

lcwangchao Nov 21, 2024

xhebox Nov 22, 2024

mjonss Nov 22, 2024

lcwangchao Nov 25, 2024

codecov bot commented Nov 20, 2024 •

edited

Loading

ti-chi-bot bot commented Nov 20, 2024

ti-chi-bot bot commented Nov 21, 2024

ti-chi-bot bot commented Nov 21, 2024

lcwangchao Nov 21, 2024

mjonss Nov 21, 2024

xhebox Nov 22, 2024

mjonss Nov 22, 2024

lcwangchao commented Nov 21, 2024

mjonss commented Nov 22, 2024

tiprow bot commented Nov 22, 2024

lcwangchao Nov 25, 2024

lcwangchao Nov 25, 2024

lcwangchao Nov 25, 2024

ddl: Fixed partitioning a non-partitioned table with placement rules #57560

Are you sure you want to change the base?

ddl: Fixed partitioning a non-partitioned table with placement rules #57560

Conversation

mjonss commented Nov 20, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 20, 2024 • edited Loading

Codecov Report

ti-chi-bot bot commented Nov 20, 2024

[LGTM Timeline notifier]

ti-chi-bot bot commented Nov 21, 2024

ti-chi-bot bot commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcwangchao commented Nov 21, 2024

mjonss commented Nov 22, 2024

tiprow bot commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjonss commented Nov 20, 2024 •

edited

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading