Uh oh!
There was an error while loading. Please reload this page.
HBASE-25880 remove files from filesCompacting when clear compaction queue - #3261
Open
frostruan wants to merge 7 commits into
Open
HBASE-25880 remove files from filesCompacting when clear compaction queue#3261frostruan wants to merge 7 commits into
frostruan wants to merge 7 commits into
Conversation
Apache-HBase
commented
May 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 12, 2021
🎊 +1 overall
This message was automatically generated. |
Contributor
There was a problem hiding this comment.
Did you see a problem from our not removing files from compacting list? Bad metrics or something?
A few notes in below. Thanks.
Oh, for the future, the subject of the PR should lead off w/ the JIRA that covers the PR. Just an FYI. You add the JIRA in the comment.... so we can fix on merge...
| } | ||
| } | ||
| public List<HStoreFile> getFilesCompacting() { |
Contributor
There was a problem hiding this comment.
This is for test only?
See hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java for how methods like this are marked with a @RestrictedApi annotation.
It looks like this method and the one above it can be package-private rather than public?
ContributorAuthor
There was a problem hiding this comment.
yes. Thanks for your advise. I will fix it.
Uh oh!
There was an error while loading. Please reload this page.
frostruan
commented
May 12, 2021
ContributorAuthor
Thanks for taking a look on my PR. For system compaction, selectNow will be false, which means files will be added to the compacting list when the compaction task is actually executed. So it is safe to just clear the workQueue of compaction thread pool executor. However, for user-triggered compaction, selectNow will be true, which means files are already added to the compacting list when we put the compaction request to the queue. When CompactionPolicy selects candidate files for minor compaction, files in compacting list will be excluded. If we just clear the workQueue of compaction thread pool executor, files of these compaction requests are still in compacting list. These files will not be selected by CompactionPolicy and compacted any more unless we force a major compaction. So I think maybe we should remove those files from compacting list when we clear the compaction queue. |
Apache-HBase
commented
May 13, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 13, 2021
🎊 +1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
guangxuCheng
requested changes
May 13, 2021
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
May 13, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 13, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 13, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 13, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 14, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 14, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 14, 2021
🎊 +1 overall
This message was automatically generated. |
saintstack
left a comment
Contributor
There was a problem hiding this comment.
Some questions but LGTM. You'll need to get the go from @guangxuCheng Thanks.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache9
reviewed
May 22, 2021
| continue; | ||
| } | ||
| CompactionRunner runner = (CompactionRunner) runnable; | ||
| if (runner.compaction != null && runner.compaction.hasSelection()) { |
Contributor
There was a problem hiding this comment.
No data race here? Not sure, just asking
ContributorAuthor
There was a problem hiding this comment.
Thanks for taking a look on this PR.
I don't think there is data race here. It's safe. Thanks.
frostruan
commented
May 28, 2021
ContributorAuthor
@Apache9@guangxuCheng hi, any questions here? |
bbeaudreault
commented
May 13, 2024
Contributor
Can we pick this back up? This is actually a big issue. If you clear compaction queues, you're almost guaranteed to enter a case where new compactions for those regions cannot proceed. Because the compaction selection will see many of the files of the Stores as already compacting, so will be skipped. |
Apache9
commented
May 13, 2024
Contributor
Have you faced this problem in your production? @bbeaudreault We can pick this up I think. |
Contributor
Yea, we just ran into this twice recently in our production. We didn't realize that compactions weren't working until a while later when we got alerted for too many storefiles. The only solution was to move the regions off then back on. I took a quick look and prior to adding to workQueue, we add to filesCompacting. So if we remove from workQueue, we have to remove from filesCompacting. Otherwise those files will never be compacted until the region moves away from the RS. |
frostruan
commented
May 14, 2024
ContributorAuthor
We have this patch in our internal branch for a long time, and the effect is in line with expectations. So I think we can pick this up too. |
frostruan
commented
May 14, 2024
ContributorAuthor
@bbeaudreault This PR has some conflicts with the master branch. I can fix PR if you need. And if you have new or more ideas, it's completely ok for me to close this PR and you can submit new one. I'll be happy to help review it. Thanks. |
bbeaudreault
commented
May 14, 2024
Contributor
Thanks @frostruan. I don't have any other feedback here, I just saw that it stalled. If you have time to finish it, that'd be great. If not I can ask someone at my company to resubmit it |
bbeaudreault
approved these changes
May 14, 2024
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
The code to me looks exactly how I'd handle it. Lgtm once merge conflict fixed and pre-commit passes
frostruan
commented
May 14, 2024
ContributorAuthor
If anyone at your company interested in this, please feel free to submit new PR. I'll be happy to help review. Thanks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When clear compaction queues, we just clear the workQueue of ThreadPoolExecutor, but files in compaction request are still in filesCompacting list. maybe we should clear it also.
https://issues.apache.org/jira/browse/HBASE-25880