ddl: Fixed partitioning a non-partitioned table with placement rules #57560
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
release-note
|
Hi @mjonss. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no suggestions.
| end string | ||
| } | ||
| keys := make([]keyRange, 0, rules) | ||
| for k := range m.bundles { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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})
}There was a problem hiding this comment.
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?
There was a problem hiding this comment.
We still need to check the rules in one bundle even
bundle[k].Override is true
true.
There was a problem hiding this comment.
@lcwangchao @xhebox please review the new duplicate leader range check.
There was a problem hiding this comment.
So, why not only validating the input bundles one by one? I think the current version does not care the overlaps across different rules.
There was a problem hiding this comment.
I have changed the logic to only check bundles one by one, and tried to mimic the logic of PDs prepareRulesForApply() and checkApplyRules(), for any other check I created #57693 as a follow up.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #57560 +/- ##
================================================
+ Coverage 72.8374% 75.2781% +2.4406%
================================================
Files 1677 1722 +45
Lines 464022 478576 +14554
================================================
+ Hits 337982 360263 +22281
+ Misses 105168 96226 -8942
- Partials 20872 22087 +1215
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ti-chi-bot
bot
added
size/L
mjonss
added
the
do-not-merge/work-in-progress
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
|
@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. |
| // - 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, |
There was a problem hiding this comment.
Why the new bundles should include old partitions? The old bundles have already defined these rules and still take effects here.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think the rule bundle check improvements would be better addressed in a separate PR, I will update the check and add a unit test here, but when looking at the code in PD, it looks like the example in the comment for prepareRulesForApply() is wrong, if I'm reading the code correctly, I would expect it to return 'ruleC and ruleD`, since ruleB would override ruleA, and ruleC would override all previous rules, due to its group has override set, and finally ruleD would just be added.
And it does not check any key ranges, neither in this function or in checkApplyRules(), which only checks that there are at least one leader or voter, and max one leader.
There was a problem hiding this comment.
The example would be correct if the rules would be ordered in reverse, i.e. according to their GroupID, ID.
There was a problem hiding this comment.
I have changed the logic to only check bundles one by one, and tried to mimic the logic of PDs prepareRulesForApply() and checkApplyRules(), for any other check I created #57693 as a follow up.
| for i := 1; i < len(keys); i++ { | ||
| if keys[i].groupID != keys[j].groupID { | ||
| // currently not checking if the group overrides all other groups! | ||
| applyKeys = append(applyKeys, keys[j:i]...) // save rules belong to previous groups | ||
| j = i | ||
| } | ||
| if keys[i].override { | ||
| j = i // skip all previous rules in the same group | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't understand these codes very well. Should we assume that all rules in the same bundle share the same group ID? If so, we just need to validate the rule.GroupID == bundle.ID
There was a problem hiding this comment.
Niether do I, so I did check what PD is implementing and tried to replicate that.
Since these are additional checks, and copied from pd, I would prefer to only have a limited check, that would still catch the main issue that is fixed in the PR, rather than also adding full check, that is not related to the original issue. I would prefer to have the more thorough check in a separate PR, like what #57693 suggests.
|
/retest |
|
@mjonss: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
| for _, rule := range bundle.Rules { | ||
| if rule.GroupID != bundle.ID { | ||
| // TODO: also check cross IDs | ||
| continue | ||
| } | ||
| keys = append(keys, keyRange{ | ||
| groupID: rule.GroupID, | ||
| id: rule.ID, | ||
| start: rule.StartKeyHex, | ||
| end: rule.EndKeyHex, | ||
| override: rule.Override, | ||
| isLeader: rule.Role == pd.Leader, | ||
| }) | ||
| } | ||
| if len(keys) == 0 { | ||
| return nil | ||
| } | ||
| // Skip overridden rules, but only within the bundle, not across groups | ||
| applyKeys := keys[:0] | ||
| j := 0 | ||
| for i := 1; i < len(keys); i++ { | ||
| if keys[i].override { | ||
| j = i // skip all previous rules in the same group | ||
| // TODO: Should we only override the matching key range? | ||
| } | ||
| } | ||
| applyKeys = append(applyKeys, keys[j:]...) | ||
| if len(applyKeys) == 0 { | ||
| return fmt.Errorf(`ERROR 8243 (HY000): "[PD:placement:ErrBuildRuleList]build rule list failed, no rule left`) | ||
| } | ||
|
|
||
| // Additionally check for range overlapping leaders. | ||
| keys = keys[:0] | ||
| for i := 0; i < len(applyKeys); i++ { | ||
| if applyKeys[i].isLeader { | ||
| keys = append(keys, applyKeys[i]) | ||
| } | ||
| } | ||
| if len(keys) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| // Sort on Start, id, end | ||
| // Could use pd's placement.sortRules() instead. | ||
| sort.Slice(keys, func(i, j int) bool { | ||
| if keys[i].start == keys[j].start { | ||
| if keys[i].id == keys[j].id { | ||
| return keys[i].end < keys[j].end | ||
| } | ||
| return keys[i].id < keys[j].id | ||
| } | ||
| return keys[i].start < keys[j].start | ||
| }) |
There was a problem hiding this comment.
Maybe we can make above logic more simple by:
for _, rule := range bundle.Rules {
if rule.GroupID != bundle.ID {
return errors.New(....)
}
if rule.Role == pd.Leader && rule.Override {
keys = append(keys, keyRange{
start: rule.StartKeyHex,
end: rule.EndKeyHex,
})
}
}
if len(keys) == 0 {
return nil
}
// Sort on Start, id, end
// Could use pd's placement.sortRules() instead.
sort.Slice(keys, func(i, j int) bool {
if keys[i].start == keys[j].start {
return keys[i].end < keys[j].end
}
return keys[i].start < keys[j].start
})
Seems prepareRulesForApply is handling rules in the same range which requires building actual rules for each range. We do not need to do it for simple. We can just check there is no overlap in non-override leader rules
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Defined2014, lcwangchao, wjhuang2016 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:
Approvers can indicate their approval by writing |
|
/retest |
|
@mjonss: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
|
/retest |
|
@mjonss: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
|
In response to a cherrypick label: new pull request created to branch |
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:
to
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
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.