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

executor: add ALTER TABLE COMPACT support #34741

Merged
merged 21 commits into from
May 18, 2022
Merged

executor: add ALTER TABLE COMPACT support #34741

merged 21 commits into from
May 18, 2022

Conversation

breezewish
Copy link
Member

@breezewish breezewish commented May 17, 2022 •
edited
Loading

Signed-off-by: Wish [email protected]

What problem does this PR solve?

Issue Number: ref #34749

Problem Summary:

What is changed and how it works?

Parser:

  • Add a new ALTER TABLE [name] COMPACT TIFLASH REPLICA syntax.

Executor:

  • A new executor for executing corresponding SQL statement.

Related TiFlash PR that supports the new RPC endpoint: pingcap/tiflash#4885

Check List

Tests

  • Unit test
  • Integration test: Will be included in the TiFlash side
  • Manual test (add detailed scripts or steps below)
    • Kill
    • Kill when there are multiple partitions
    • One TiFlash down
    • No TiFlash is up
    • Multiple partitions
  • No code

Side effects

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

Documentation

  • Affects user behaviors
  • Contains syntax changes - A new syntax is introduced, see above.
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility - This statement is not available in MySQL.

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Support ALTER TABLE COMPACT statement

@ti-chi-bot
Copy link
Member

ti-chi-bot commented May 17, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • XuHuaiyu
  • crazycs520

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 do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 17, 2022
@sre-bot
Copy link
Contributor

sre-bot commented May 17, 2022 •
edited
Loading

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/e94b2d24886e3e9be535ee88c7f68b6678f257ca

@ti-chi-bot ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/needs-linked-issue labels May 17, 2022
@breezewish
Merge upstream
d11260d 
Signed-off-by: Wish <[email protected]>
@breezewish breezewish mentioned this pull request May 18, 2022
Merged
12 tasks
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2022
crazycs520
crazycs520 approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 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 the status/LGT1 Indicates that a PR has LGTM 1. label May 18, 2022
lidezhu
lidezhu approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@lidezhu lidezhu 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
Copy link
Member

@lidezhu: 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

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.

@breezewish
Add unit tests
2f20001 
Signed-off-by: Wish <[email protected]>
@ti-chi-bot ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label May 18, 2022
@breezewish breezewish changed the title [WIP] executor: add ALTER TABLE COMPACT support executor: add ALTER TABLE COMPACT support May 18, 2022
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 18, 2022
@breezewish breezewish added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2022
@breezewish
Copy link
Member Author

breezewish commented May 18, 2022 •
edited
Loading

Add DNM label since this PR must be merged after pingcap/tiflash#4885. This PR does not rely on that PR to pass tests. Merging in parallel.

ichn-hu
ichn-hu reviewed May 18, 2022
View reviewed changes
executor/compact_table.go Outdated Show resolved Hide resolved
ichn-hu
ichn-hu reviewed May 18, 2022
View reviewed changes
Copy link
Contributor

@ichn-hu ichn-hu left a comment

Choose a reason for hiding this comment

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

generally LGTM with some comments

executor/compact_table.go Show resolved Hide resolved
@ti-chi-bot
Copy link
Member

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

Commit hash: 9a0cac5

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 18, 2022
ichn-hu
ichn-hu approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@ichn-hu ichn-hu left a comment

Choose a reason for hiding this comment

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

the code is clean, LGTM

@breezewish breezewish removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2022
@breezewish breezewish removed the status/can-merge Indicates a PR has been approved by a committer. label May 18, 2022
@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 18, 2022
@breezewish
Copy link
Member Author

breezewish commented May 18, 2022 •
edited
Loading

The unit test is 100% failed due to tikv/client-go#473. Pause the merging to save some CI resource.

Isolated test: #34789 (which upgrades client-go only)

Update: Rolled in tikv/client-go#507 in this PR now. Unit tests shall be passed.

@breezewish breezewish removed the status/can-merge Indicates a PR has been approved by a committer. label May 18, 2022
@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 18, 2022
@breezewish breezewish added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2022
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label May 18, 2022
@breezewish breezewish removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2022
@breezewish
Copy link
Member Author

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: e94b2d2

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 18, 2022
@breezewish breezewish mentioned this pull request May 18, 2022
Closed
12 tasks
@ti-chi-bot ti-chi-bot merged commit 01fb935 into pingcap:master May 18, 2022
@breezewish breezewish deleted the wenxuan/alter-compact-table branch May 18, 2022 16:45
@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2022

TiDB MergeCI notify

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci/integration-cdc-test 🟢 all 34 tests passed 25 min Existing passed
idc-jenkins-ci-tidb/integration-common-test 🟢 all 11 tests passed 22 min Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 14 min Existing passed
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 14 min Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 14 min Existing passed
idc-jenkins-ci-tidb/common-test 🟢 all 12 tests passed 14 min Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 11 min Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 4 min 59 sec Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test 🟢 all 1 tests passed 3 min 5 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

This was referenced May 20, 2022
Closed
Merged
This was referenced Jul 13, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidezhu lidezhu lidezhu approved these changes

@ichn-hu ichn-hu ichn-hu approved these changes

@crazycs520 crazycs520 crazycs520 approved these changes

@XuHuaiyu XuHuaiyu XuHuaiyu 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. size/XXL Denotes a PR that changes 1000+ 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.
7 participants
@breezewish @ti-chi-bot @sre-bot @XuHuaiyu @crazycs520 @ichn-hu @lidezhu