-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: support REORGANIZE PARTITION, part 1 - data reorg / data+index copying #38460
*: support REORGANIZE PARTITION, part 1 - data reorg / data+index copying #38460
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
/run-all-test |
/run-build plugin=main |
/run-check_dev_2 plugin=main |
/run-build plugin=master |
1 similar comment
/run-build plugin=master |
/run-build |
/run-check_dev_2 |
/run-unit-test |
/run-check_dev_2 |
/run-unit-test |
@@ -939,19 +950,34 @@ func (t *partitionedTable) PartitionExpr() (*PartitionExpr, error) { | |||
return t.partitionExpr, nil | |||
} | |||
|
|||
func (t *partitionedTable) GetPartitionColumnNames() []model.CIStr { | |||
func (t *partitionedTable) GetPartitionColumnIDs() []int64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add a UT for this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is used by GetPartitionColumnNames, and that is included in one UT, I don't think it is needed. But if you like I can create such UT?
/run-check_dev_2 |
ddl/backfilling.go
Outdated
if err != nil { | ||
if b.canSkipError(err) { | ||
continue | ||
} | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The errors returned from newReorgPartitionWorker
are not recoverable... We had better not to skip them :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I agree and have removed it. But I don't think it would be too bad, since it would just limit the number of workers if by somehow the newReorgPartitionWorker
would have succeeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When newReorgPartitionWorker
is possible to succeed for part of workers(for example the memory is limited by the resource manager), we can add it back and update the log message inside canSkipError
.
…nto reorg-part-merge-dec22
…nto reorg-part-data-reorg
/run-unit-test |
1 similar comment
/run-unit-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 0eab90d
|
What problem does this PR solve?
First part of REORGANIZE PARTITION, implementing the data copying from one set of partitions to a new set of partitions
as in:
ALTER TABLE t REORGANIZE PARTITION INTO ()
This part covers:
Issue Number: ref #15000 , ref #38535
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.