HBASE-28654 Support only remove expired files in the compact process #6008
+62
−1
Conversation
This file contains 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. |
|
💔 -1 overall
This message was automatically generated. |
|
I prefer we change the config name to something like 'compaction.skip-merging-compact', to indicate that this config is used to skip the real compaction operation. As even if we set this config to true, there are still other options which could make us not remove expired files, such as 'hbase.store.delete.expired.storefile'. We should carefully document these behaviors. |
|
You can also send a discuss email to dev list, to see if there are better names for this config, since I'm not a native English speaker... |
1458451310
force-pushed
the
HBASE-28654
branch
from
cd969e0 to
a9f756b
Compare
OK. I have changed and add logs for the config 'hbase.store.delete.expired.storefile' is false. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
NihalJain
reviewed
Sep 4, 2024
| * true. If flag is false then do normal compactions. | ||
| * @return true if table compaction only remove expired file, skipping merge files. | ||
| */ | ||
| boolean isSkipMergingCompaction(); |
There was a problem hiding this comment.
shouldSkipMergingCompaction make more sense to me over isSkipMergingCompaction
| @@ -1453,6 +1453,15 @@ public Optional<CompactionContext> requestCompaction(int priority, | |||
| // Before we do compaction, try to get rid of unneeded files to simplify things. | |||
| removeUnneededFiles(); | |||
|
|
|||
| if (this.region.getTableDescriptor().isSkipMergingCompaction()) { | |||
| if (!conf.getBoolean("hbase.store.delete.expired.storefile", true)) { | |||
| LOG.info("Because the conf of hbase.store.delete.expired.storefile is false, so the table " | |||
There was a problem hiding this comment.
Can we rephrase as "Since config 'hbase.store.delete.expired.storefile' is set to false, ignoring SKIP_MERGING_COMPACTION set at table level"
| @@ -85,6 +85,15 @@ public class TableDescriptorBuilder { | |||
| public static final String COMPACTION_ENABLED = "COMPACTION_ENABLED"; | |||
| private static final Bytes COMPACTION_ENABLED_KEY = new Bytes(Bytes.toBytes(COMPACTION_ENABLED)); | |||
|
|
|||
| /** | |||
| * Used by HBase Shell interface to access this metadata attribute which denotes if the table | |||
There was a problem hiding this comment.
Are we planning to expose this via shell as another commit?
There was a problem hiding this comment.
Now, we can use shell to change the table description with SKIP_MERGING_COMPACTION.
alter 'tableName',CONFIGURATION => {'SKIP_MERGING_COMPACTION' => 'true'}
NihalJain
reviewed
Sep 4, 2024
| @@ -1453,6 +1453,15 @@ public Optional<CompactionContext> requestCompaction(int priority, | |||
| // Before we do compaction, try to get rid of unneeded files to simplify things. | |||
| removeUnneededFiles(); | |||
|
|
|||
| if (this.region.getTableDescriptor().isSkipMergingCompaction()) { | |||
| if (!conf.getBoolean("hbase.store.delete.expired.storefile", true)) { | |||
There was a problem hiding this comment.
Also could you refactor 'hbase.store.delete.expired.storefile' into a variable along with its default value? I see only one usage of this in the code base, apart from testcode.
There was a problem hiding this comment.
Also could you refactor 'hbase.store.delete.expired.storefile' into a variable along with its default value? I see only one usage of this in the code base, apart from testcode.
I have changed based on your suggestion. Please review again when you have time.
1458451310
force-pushed
the
HBASE-28654
branch
from
a9f756b to
80c7b26
Compare
1458451310
force-pushed
the
HBASE-28654
branch
from
80c7b26 to
4665a54
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://issues.apache.org/jira/browse/HBASE-28654