HBASE-26987 The length of compact queue grows too big when the compac… #4390
+153
−39
Conversation
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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
rebase |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
May 5, 2022
| final LongAdder compactionsFailed = new LongAdder(); | ||
| final LongAdder compactionNumFilesCompacted = new LongAdder(); | ||
| final LongAdder compactionNumBytesCompacted = new LongAdder(); | ||
| final LongAdder compactionsQueued = new LongAdder(); |
There was a problem hiding this comment.
It seems a bit strange that we only move this field to HStore but still keep other compaction related metrics in HRegion...
There was a problem hiding this comment.
Yeah, you are right, i thought about it too, but did not find a better way, maybe all that metrics should be moved to HStore?
Thanks.
There was a problem hiding this comment.
I think moving compaction metrics to HStore would be better.
There was a problem hiding this comment.
I think moving compaction metrics to HStore would be better.
I can move the other compaction metrics in a follow-on issue.
There was a problem hiding this comment.
Then is it possible to not include the metrics moving but still fix the problem here? It is not a good idea to land a half done work, so I prefer either we do not move any of them, or move them all at once.
Yeah, the problem here is we put too many compactions to queue, so i think the solution for it is to find how many is suitable. |
virajjasani
reviewed
May 18, 2022
There was a problem hiding this comment.
The idea looks good overall. @bsglz do you have any recommendation on the max files to compact value?
| final LongAdder compactionsFailed = new LongAdder(); | ||
| final LongAdder compactionNumFilesCompacted = new LongAdder(); | ||
| final LongAdder compactionNumBytesCompacted = new LongAdder(); | ||
| final LongAdder compactionsQueued = new LongAdder(); |
There was a problem hiding this comment.
I think moving compaction metrics to HStore would be better.
Do you mean the value for hbase.hstore.compaction.max? The default value is 10, i think its ok. |
|
+1 overall, also the idea seems good. @bsglz let's wait for one more review? |
No prob, thanks a lot. |
|
@Apache9 Can i have +1 from you, or any more issues? Thanks a lot. |
Apache9
reviewed
Jun 1, 2022
|
|
||
| @Override | ||
| public boolean needsCompaction() { | ||
| // For some system compacting, we set selectNow to false, and the files do not |
There was a problem hiding this comment.
In general I do not fully understand what is the problem here...
The problem is we schedule too many compactions, because of how we know whether there is a compaction running is through the filesCompacting? This is only true for system compaction? Then why not change the logic of requesting system compaction?
There was a problem hiding this comment.
The current logic is an improvement introduced by HBASE-8665, which could get better store file selection.
I think that patch is useful, just bring some side effect, and this patch want to fix it.
BTW, skimming that patch maybe helpful to understand.
Thanks a lot.
|
rebase |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Ping @Apache9 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
…ting is slow