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

ddl: handle placement rule cache for drop/truncate/recover/flashback table #20622

Merged
merged 15 commits into from
Nov 2, 2020
Merged

ddl: handle placement rule cache for drop/truncate/recover/flashback table #20622

merged 15 commits into from
Nov 2, 2020

Conversation

xhebox
Copy link
Contributor

@xhebox xhebox commented Oct 25, 2020 •
edited
Loading

What problem does this PR solve?

Problem Summary: #20575 has added the function that lazily delete placement rules on PD when tables are removed by GC. This PR completes the function:

  1. Drop rules when drop/truncate table, but only drop rule cache. Real rules still exist on PD, and they are deleted when gc.
  2. Truncate table will post new placement rules on PD.
  3. Recover/Flashback table will pull deleted rules from PD back to rule cache

Check List

Tests

  • Unit test
  • Integration test

There should be a manual test though.. Maybe wait for PRs that adds SQL statements to modify rules on Table.

Release note

  • No release note

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Oct 25, 2020
@xhebox xhebox marked this pull request as draft October 25, 2020 09:49
@xhebox
Copy link
Contributor Author

xhebox commented Oct 25, 2020

@ti-srebot /run-all-tests

@xhebox xhebox marked this pull request as ready for review October 25, 2020 11:46
@ti-srebot
Copy link
Contributor

/run-all-tests

@xhebox xhebox mentioned this pull request Oct 26, 2020
Merged
djshow832
djshow832 reviewed Oct 27, 2020
View reviewed changes
ddl/ddl_worker.go Outdated Show resolved Hide resolved
ddl/ddl_worker.go Outdated Show resolved Hide resolved
infoschema/builder.go Show resolved Hide resolved
ddl/table.go Outdated Show resolved Hide resolved
ddl/table.go Show resolved Hide resolved
djshow832
djshow832 reviewed Oct 28, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

Please add test cases by the following procedure:
create a table with partitions -> mock rule cache -> drop the table -> check the rule cache directly or by information_schema.placement_policy.

ddl/ddl_worker.go

affects := make([]*model.AffectedOption, len(oldIDs))
for i := 0; i < len(oldIDs); i++ {
affects[i] = &model.AffectedOption{
Copy link
Contributor

Choose a reason for hiding this comment

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

What about schemaID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not used anyway. Affects are processed by L141-165, then continue the loop. It is never passed to ApplyDiff or whatever function that required schemaID.

ddl/placement/utils.go Outdated Show resolved Hide resolved
xhebox and others added 2 commits October 28, 2020 11:39
@xhebox
Copy link
Contributor Author

xhebox commented Oct 29, 2020

@ti-srebot /run-all-tests

@ti-srebot
Copy link
Contributor

/run-all-tests

@xhebox
ddl: lazily update rule cache when truncate
4417a58 
not every partition needs to update, some partitions do not have a
non-empty bundle.

Signed-off-by: xhe <[email protected]>
@xhebox
Copy link
Contributor Author

xhebox commented Oct 29, 2020

@ti-srebot /run-all-tests

@ti-srebot
Copy link
Contributor

/run-all-tests

djshow832
djshow832 reviewed Oct 30, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 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-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 30, 2020
tangenta
tangenta reviewed Nov 2, 2020
View reviewed changes
Copy link
Contributor

@tangenta tangenta 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-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 2, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 2, 2020
@xhebox
Copy link
Contributor Author

xhebox commented Nov 2, 2020

@ti-srebot /merge

@ti-srebot
Copy link
Contributor

@xhebox No command or invalid command

@djshow832
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 2, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit c5d0f40 into pingcap:master Nov 2, 2020
@xhebox xhebox mentioned this pull request Jul 7, 2021
Closed
This was referenced Jul 29, 2021
Closed
Closed
@xhebox xhebox mentioned this pull request Aug 9, 2021
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djshow832 djshow832 djshow832 left review comments

@tangenta tangenta tangenta left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@AilinKid AilinKid Awaiting requested review from AilinKid

Assignees
No one assigned
Labels
sig/sql-infra SIG: SQL Infra 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.
4 participants
@xhebox @ti-srebot @djshow832 @tangenta