Skip to content
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

executor, ddl: support database placement option and partition placement option #28025

Merged

Conversation

zhaoxugang
Copy link
Contributor

@zhaoxugang zhaoxugang commented Sep 14, 2021

What problem does this PR solve?

Issue Number: refer to #26581

Depends on parser PR pingcap/parser#1342

This closes #27973 .

Check List

Tests

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

Side effects

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

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 14, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • mjonss
  • 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.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 14, 2021
@zhaoxugang
Copy link
Contributor Author

/cc @xhebox @AilinKid

Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM

ddl/db_test.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/placement_policy_test.go Outdated Show resolved Hide resolved
ddl/placement_policy_test.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I missed this and did some duplicated work (but good for my own understanding) here:
#27969

ddl/ddl_api.go Outdated
var opt *ast.CharsetOpt
if len(dbOps) != 0 {
opt = &ast.CharsetOpt{}
for _, val := range dbOps {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it is duplicated and would benefit from a common function, similar to this:
https://github.com/pingcap/tidb/pull/27969/files#diff-b7c54f7dad7257cbe4fa04b8d76c9bed694e2e2e7c5599f45eb2172d8d4ff8a7R2289

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it is duplicated and would benefit from a common function, similar to this:
https://github.com/pingcap/tidb/pull/27969/files#diff-b7c54f7dad7257cbe4fa04b8d76c9bed694e2e2e7c5599f45eb2172d8d4ff8a7R2289

I will revoke the part of schema and use the common func that named SetDirectPlacementOpt to simple code after your pr be merged .

@mjonss mjonss mentioned this pull request Sep 15, 2021
10 tasks
Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 17, 2021
Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pingcap/parser#1342 has been merged, you could update the parser and remove replace directives.

@ti-chi-bot 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 22, 2021
@zhaoxugang zhaoxugang force-pushed the support_database_placement_option_ branch from bfeedc2 to 483b82b Compare September 22, 2021 15:37
@ti-chi-bot 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 Sep 22, 2021
@zhaoxugang
Copy link
Contributor Author

/rebuild

@zhaoxugang
Copy link
Contributor Author

/run-check_dev_2

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

@mjonss mjonss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one suggestion below.

ddl/table.go Outdated
job.State = model.JobStateCancelled
return 0, errors.Trace(table.ErrUnknownPartition.GenWithStackByArgs("drop?", tblInfo.Name.O))
}
for idx, ptDef := range ptInfo.Definitions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at (*PartitionInfo) GetNameByID, it looks like this could be done similarly, including the check for existence above.
I.e. could you remove the if ptInfo.GetNameByID(partitionID) ... and replace the loop here with for i := range ptInfo.Definitions { and then check afterwards if the partition was found and if not return the error? Then you will only iterate over the array once, and according to the note in GetNameByID it is also faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@ti-chi-bot
Copy link
Member

@mjonss: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM with one suggestion below.

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 ti-community-infra/tichi repository.

@morgo
Copy link
Contributor

morgo commented Sep 28, 2021

LGTM also, but I will wait for @mjonss ' comment to be answered before approving/merging.

Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot 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 1, 2021
@tisonkun
Copy link
Contributor

tisonkun commented Oct 5, 2021

@mjonss & @xhebox does this PR need a release note? At least current release note is not a proper one. Please also take care of the release note during a review.

@tisonkun
Copy link
Contributor

tisonkun commented Oct 5, 2021

Also it closes a closed issue #26581, but why?

cc @morgo

@xhebox
Copy link
Contributor

xhebox commented Oct 6, 2021

@mjonss & @xhebox does this PR need a release note? At least current release note is not a proper one. Please also take care of the release note during a review.

We could a release note in other PRs. There are too many PRs related to the feature.

Also it closes a closed issue #26581, but why?

I think it is a mistake by @zhaoxugang , he may want to say 'refer' or 'related'.

@zhaoxugang
Copy link
Contributor Author

@xhebox @tisonkun

@mjonss & @xhebox does this PR need a release note? At least current release note is not a proper one. Please also take care of the release note during a review.

We could a release note in other PRs. There are too many PRs related to the feature.

Also it closes a closed issue #26581, but why?

I think it is a mistake by @zhaoxugang , he may want to say 'refer' or 'related'.

I have fixed it

@ti-chi-bot ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 6, 2021
@tisonkun
Copy link
Contributor

tisonkun commented Oct 6, 2021

@zhaoxugang thanks. I've updated the release note part to None for you. @xhebox please check whether we write the release note somewhere so that we don't miss this feature in 5.3.0 release.

@zhaoxugang please fix go.mod conflict.

/assign @xhebox

Can we merge this PR later or we still wait for another committer's approval?

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2021
@xhebox
Copy link
Contributor

xhebox commented Oct 8, 2021

@zhaoxugang thanks. I've updated the release note part to None for you. @xhebox please check whether we write the release note somewhere so that we don't miss this feature in 5.3.0 release.

@zhaoxugang please fix go.mod conflict.

/assign @xhebox

Can we merge this PR later or we still wait for another committer's approval?

No, it can be merged now. I believe the release note is included in #27969

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: c3d6792

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

@zhaoxugang: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure the partitionDef options for placement are persisted in TiDB DDL
6 participants