Skip to content

executor: add ALTER TABLE COMPACT support#34741

Merged
ti-chi-bot merged 21 commits intopingcap:masterfrom
breezewish:wenxuan/alter-compact-table
May 18, 2022
Merged

executor: add ALTER TABLE COMPACT support#34741
ti-chi-bot merged 21 commits intopingcap:masterfrom
breezewish:wenxuan/alter-compact-table

Conversation

@breezewish
Copy link
Member

@breezewish breezewish commented May 17, 2022 •
edited
Loading

Signed-off-by: Wish breezewish@outlook.com

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

@breezewish
executor: add ALTER TABLE [name] COMPACT TIFLASH REPLICA support
161c685 
Signed-off-by: Wish <breezewish@outlook.com>
@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.

Details

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
@breezewish
Use a remote client-go
0e28459 
Signed-off-by: Wish <breezewish@outlook.com>
@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 <breezewish@outlook.com>
@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
@breezewish
Remove the replace directive
8851093 
Signed-off-by: Wish <breezewish@outlook.com>
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

ti-chi-bot commented May 18, 2022

@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.

Details

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 <breezewish@outlook.com>
@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
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

@ti-chi-bot
Copy link
Member

ti-chi-bot commented May 18, 2022

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

DetailsCommit 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
@breezewish
upgrade client-go again
da65868 
Signed-off-by: Wish <breezewish@outlook.com>
@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

breezewish commented May 18, 2022

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented May 18, 2022

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

DetailsCommit 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

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

@crazycs520 crazycs520 crazycs520 approved these changes

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

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