HBASE-26320 Implement a separate thread pool for the LogCleaner #3712
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. |
Apache9
reviewed
Oct 2, 2021
There was a problem hiding this comment.
The PR is straight forward, in general I think it is fine to have different scan pools for wal and hfile, since they may on different storage types, where the speed for scanning is different.
Only have some concerns on the naming so left some comments, please take a look.
Thanks,
| @@ -367,7 +367,8 @@ | |||
|
|
|||
| private HbckChore hbckChore; | |||
| CatalogJanitor catalogJanitorChore; | |||
| private DirScanPool cleanerPool; | |||
| private DirScanPool archiveCleanerPool; | |||
There was a problem hiding this comment.
Better add some comments here? I think the logCleanerPool is for removing wals, and the archiveCleanerPool is for all other cleaners? And do we need DirScanPool for cleaners other than log and hfile? If not, let's just name them logCleanerPool and hfileCleaner pool?
|
|
||
| public DirScanPool(Configuration conf) { | ||
| String poolSize = conf.get(CleanerChore.CHORE_POOL_SIZE, CleanerChore.DEFAULT_CHORE_POOL_SIZE); | ||
| private DirScanPool(Configuration conf, String cleanerPoolConfig, String cleanerPoolConfigDefault, |
There was a problem hiding this comment.
CleanerPoolConfig is too general and I thought it was a class...
Let's just name them cleanerPoolSizeConfigName, and cleanerPoolSizeConfigDefault?
There was a problem hiding this comment.
+1 for this suggested name. Looks clean.
Can we think of making this more clean?
DirScanPool can have a private Enum Type (for ArchivedHFilesCleaner, OldLogsCleaner)
The constructor can only take the type. So the other details of name, configName, configDefault are considered based on type. Infact type can have a method to return the poolSize.
We dont have to even store the cleanerPoolConfigDefault
I think its used only on configUpdate method. That can use Configuration#get(String) and have a null check?
There was a problem hiding this comment.
+1 for "DirScanPool can have a private Enum Type (for ArchivedHFilesCleaner, OldLogsCleaner)"
| pool.setCorePoolSize(size); | ||
| } | ||
|
|
||
| public int getSize() { | ||
| return size; | ||
| } | ||
|
|
||
| public static DirScanPool getArchiveScanner(Configuration conf) { |
There was a problem hiding this comment.
GetScanPool instead of getScanner? Scanner has special meanings in HBase which may confuse developers.
There was a problem hiding this comment.
+1. Got the same Q when read the method name at begin of patch.
| CleanerChore.DEFAULT_CHORE_POOL_SIZE, "Archive"); | ||
| } | ||
|
|
||
| public static DirScanPool getOldLogsScanner(Configuration conf) { |
anoopsjohn
reviewed
Oct 4, 2021
| @@ -64,6 +64,8 @@ | |||
| */ | |||
| public static final String CHORE_POOL_SIZE = "hbase.cleaner.scan.dir.concurrent.size"; | |||
| static final String DEFAULT_CHORE_POOL_SIZE = "0.25"; | |||
| public static final String LOG_CLEANER_CHORE_SIZE = "hbase.log.cleaner.scan.dir.concurrent.size"; | |||
There was a problem hiding this comment.
So now we will have (by default) a pool of 1 thread for scan and cleaning of Archived HFiles.
For old WALs the thread pool's default size will be 25% of #cores
Can we add some code level comments here?
There was a problem hiding this comment.
I think, pool of 1 thread (by default) is for scanning and cleaning the old WALs. And pool with 25% of #cores (by default) is for archived store files.
+1 adding code level comments.
|
|
||
| public DirScanPool(Configuration conf) { | ||
| String poolSize = conf.get(CleanerChore.CHORE_POOL_SIZE, CleanerChore.DEFAULT_CHORE_POOL_SIZE); | ||
| private DirScanPool(Configuration conf, String cleanerPoolConfig, String cleanerPoolConfigDefault, |
There was a problem hiding this comment.
+1 for this suggested name. Looks clean.
Can we think of making this more clean?
DirScanPool can have a private Enum Type (for ArchivedHFilesCleaner, OldLogsCleaner)
The constructor can only take the type. So the other details of name, configName, configDefault are considered based on type. Infact type can have a method to return the poolSize.
We dont have to even store the cleanerPoolConfigDefault
I think its used only on configUpdate method. That can use Configuration#get(String) and have a null check?
| pool.setCorePoolSize(size); | ||
| } | ||
|
|
||
| public int getSize() { | ||
| return size; | ||
| } | ||
|
|
||
| public static DirScanPool getArchiveScanner(Configuration conf) { |
There was a problem hiding this comment.
+1. Got the same Q when read the method name at begin of patch.
|
Thanks @Apache9, @anoopsjohn and @pankaj72981 for taking a look. I have updated based on your suggestions. |
|
🎊 +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
Oct 8, 2021
hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/DirScanPool.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/DirScanPool.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/DirScanPool.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/master/region/MasterRegionTestBase.java
Outdated
Show resolved
Hide resolved
This avoids starvation when the archive directory is large and takes a long time to iterate through.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
The unit test failures don't seem related and only fail on Java 11 it seems. @Apache9 Do you have any further comments? Thanks |
Apache9
approved these changes
Oct 14, 2021
anoopsjohn
approved these changes
Oct 15, 2021
pankaj72981
approved these changes
Oct 18, 2021
|
Thanks for all the reviews! |
sunhelly
pushed a commit
to sunhelly/hbase
that referenced
this pull request
May 24, 2022
…he#3712) This avoids starvation when the archive directory is large and takes a long time to iterate through. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Pankaj <[email protected]>
sunhelly
pushed a commit
to sunhelly/hbase
that referenced
this pull request
May 24, 2022
…he#3712) This avoids starvation when the archive directory is large and takes a long time to iterate through. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Pankaj <[email protected]>
sunhelly
added a commit
that referenced
this pull request
May 24, 2022
… (#4460) This avoids starvation when the archive directory is large and takes a long time to iterate through. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Pankaj <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
May 24, 2022
… (#4460) This avoids starvation when the archive directory is large and takes a long time to iterate through. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Pankaj <[email protected]>
petersomogyi
pushed a commit
to petersomogyi/hbase
that referenced
this pull request
Nov 23, 2022
…he#3712) (apache#4460) This avoids starvation when the archive directory is large and takes a long time to iterate through. Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Pankaj <[email protected]>
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.
This avoids starvation when the archive directory is large and takes a long time
to iterate through.