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.

Sign up for GitHub

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

Jump to bottom

*: Support SHOW PLACEMENT for placement policies #27531

Merged
merged 20 commits into from
Aug 31, 2021
Merged

*: Support SHOW PLACEMENT for placement policies #27531

merged 20 commits into from
Aug 31, 2021

Conversation

lcwangchao
Copy link
Collaborator

@lcwangchao lcwangchao commented Aug 24, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: see #26582

Support SHOW PLACEMENT for placement policies

What is changed and how it works?

  • Move some settings to a standalone struct PlacementSettings for reuse
  • Finish SHOW PLACEMENT for placement policies

Check List

Tests

  • Unit test

Release note 

dml: support show placement

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 24, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • AilinKid
  • morgo

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-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 24, 2021
@lcwangchao lcwangchao changed the title *: Support SHOW PLACEMENT for placement policies WIP: *: Support SHOW PLACEMENT for placement policies Aug 24, 2021
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2021
@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Aug 24, 2021
@ti-chi-bot ti-chi-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 Aug 30, 2021
@lcwangchao lcwangchao changed the title WIP: *: Support SHOW PLACEMENT for placement policies *: Support SHOW PLACEMENT for placement policies Aug 30, 2021
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2021
@lcwangchao
Copy link
Collaborator Author

/run-check_dev_2

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 30, 2021
@lcwangchao
Copy link
Collaborator Author

/run-check_dev_2

@lcwangchao
Copy link
Collaborator Author

/run-check_dev_2

1 similar comment
@lcwangchao
Copy link
Collaborator Author

/run-check_dev_2

@lcwangchao
Copy link
Collaborator Author

/run-check_dev_2

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 31, 2021
@lcwangchao
Copy link
Collaborator Author

/run-check_dev_2

1 similar comment
@lcwangchao
Copy link
Collaborator Author

/run-check_dev_2

xhebox
xhebox reviewed Aug 31, 2021
View reviewed changes
infoschema/infoschema.go Outdated
@@ -406,6 +408,13 @@ func (is *infoSchema) SetBundle(bundle *placement.Bundle) {
is.ruleBundleMap[bundle.ID] = bundle
}

func (is *infoSchema) AllPlacementPolicies() (policies []*placementpolicy.PolicyInfo) {
for _, p := range is.policyMap {
Copy link
Contributor

Choose a reason for hiding this comment

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

policyMutex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After reading the current code, I found AllPlacementPolicies is not needed any more because PlacementPolicies did the same work. I'll remove this method and add PlacementPolicies to the interface.

But I think it is still safe without policyMutex because infoschema is immutable. The happens-before condition can be established for our current implement

Copy link
Contributor

Choose a reason for hiding this comment

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

It is just there in case, IMO. You are right that sometimes we are too cautious to do some great works. But this case is a little bit aggressive for me, especially that other policy APIs are following the conract of the mutex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. Actually I did not see policyMutex and just follow what the function AllSchemas did. As other policy APIs use policyMutex I think we should follow it either.

executor/show_placement.go Show resolved Hide resolved
util/placementpolicy/policy.go Outdated Show resolved Hide resolved
@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 Aug 31, 2021
@AilinKid
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 2d23b1d

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 31, 2021
@ti-chi-bot ti-chi-bot merged commit 376e85b into pingcap:master Aug 31, 2021
@lcwangchao lcwangchao deleted the show_policy_placement branch August 31, 2021 09:45
@lcwangchao lcwangchao mentioned this pull request Sep 1, 2021
Closed
5 tasks
This was referenced Sep 7, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xhebox xhebox xhebox left review comments

@AilinKid AilinKid AilinKid approved these changes

@morgo morgo morgo approved these changes

Assignees
No one assigned
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/XL Denotes a PR that changes 500-999 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lcwangchao @ti-chi-bot @AilinKid @morgo @xhebox