[GitHub] [hbase] anoopsjohn commented on a change in pull request #3712: HBASE-26320 Implement a separate thread pool for the LogCleaner

[GitBox <gi...@apache.org>]

anoopsjohn commented on a change in pull request #3712:
URL: https://github.com/apache/hbase/pull/3712#discussion_r721030034



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java
##########
@@ -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";

Review 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?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/DirScanPool.java
##########
@@ -109,11 +117,21 @@ synchronized void tryUpdatePoolSize(long timeout) {
         break;
       }
     }
-    LOG.info("Update chore's pool size from {} to {}", pool.getPoolSize(), size);
+    LOG.info("Update {} chore's pool size from {} to {}", name, pool.getPoolSize(), size);
     pool.setCorePoolSize(size);
   }
 
   public int getSize() {
     return size;
   }
+
+  public static DirScanPool getArchiveScanner(Configuration conf) {

Review comment:
       +1. Got the same Q when read the method name at begin of patch.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/DirScanPool.java
##########
@@ -39,21 +39,28 @@
   private final ThreadPoolExecutor pool;
   private int cleanerLatch;
   private boolean reconfigNotification;
+  private final String cleanerPoolConfig;
+  private final String cleanerPoolConfigDefault;
+  private final String name;
 
-  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,

Review 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?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org