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

Do background compaction when the ratio of stable data covered by delete range is too large #2416

Merged
merged 32 commits into from
Aug 23, 2021
Merged

Do background compaction when the ratio of stable data covered by delete range is too large #2416

merged 32 commits into from
Aug 23, 2021

Conversation

lidezhu
Copy link
Contributor

@lidezhu lidezhu commented Jul 16, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: close #1298

Problem Summary: https://tidb.wiki.pingcap.net/view/1Nsg3Y0xGFzl3CArCF1KJFO5zg7FhrwzDO9Lwx_kdkd0

What is changed and how it works?

Proposal: https://docs.google.com/document/d/1EHPHtxALWFtrGIkQuRqfBK461gcudn3JrgA0NOVi8OA/edit?usp=sharing

What's Changed:

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch:

Check List

Tests

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

Side effects

Release note

  • Do background compaction when the ratio of stable data covered by delete range is too large.

JaySon-Huang and others added 4 commits July 7, 2021 15:17
@JaySon-Huang
Add tests to ensure the behavior of DeleteRange
aa3dca4 
Add task-defined interval for background task
@JaySon-Huang
Cleanup some unreasonable include
e0d27b0 
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
@JaySon-Huang
Use macro to define enums
6177426 
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
@lidezhu
Merge branch 'master' into compact_by_delete_range
9c4f3a6
@JaySon-Huang
Copy link
Contributor

Maybe we should separate the changes in BackgroundProcessingPool (and related changes) into another PR. They are not strongly related to the "compact by delete range".

@lidezhu
Copy link
Contributor Author

lidezhu commented Jul 19, 2021

Maybe we should separate the changes in BackgroundProcessingPool (and related changes) into another PR. They are not strongly related to the "compact by delete range".

Good idea. I will do it after finishing test.

@lidezhu
Copy link
Contributor Author

lidezhu commented Aug 4, 2021

Separate background task pool change in this pr: #2571

@ti-chi-bot ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 12, 2021
@lidezhu lidezhu changed the title [WIP]Compact by delete range Do background gc when the ratio of stable data covered by delete range exceeds some ratio Aug 12, 2021
@ti-chi-bot
Copy link
Member

@lidezhu: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2021
@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 Aug 23, 2021
@lidezhu
Copy link
Contributor Author

lidezhu commented Aug 23, 2021

/run-all-tests

@flowbehappy flowbehappy self-requested a review August 23, 2021 09:05
flowbehappy
flowbehappy approved these changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

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

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • flowbehappy

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 the status/LGT1 Indicates that a PR has LGTM 1. label Aug 23, 2021
@lidezhu
Copy link
Contributor Author

lidezhu commented Aug 23, 2021

/merge

@ti-chi-bot
Copy link
Member

@lidezhu: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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.

@ti-chi-bot
Copy link
Member

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

Commit hash: 4e7a39b

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 23, 2021
JaySon-Huang
JaySon-Huang approved these changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

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

LGTM

@lidezhu lidezhu removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 23, 2021
@ti-chi-bot ti-chi-bot merged commit 8802977 into pingcap:master Aug 23, 2021
@lidezhu lidezhu deleted the compact_by_delete_range branch August 23, 2021 11:06
This was referenced Dec 21, 2021
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flowbehappy flowbehappy flowbehappy approved these changes

@JaySon-Huang JaySon-Huang JaySon-Huang approved these changes

Assignees

@lidezhu lidezhu

Labels
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/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Predicable data cleanup strategies after Regions peers are removed
4 participants
@lidezhu @JaySon-Huang @ti-chi-bot @flowbehappy