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

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

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



##########
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:
       CleanerPoolConfig is too general and I thought it was a class...
   
   Let's just name them cleanerPoolSizeConfigName, and cleanerPoolSizeConfigDefault?

##########
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) {
+    return new DirScanPool(conf, CleanerChore.CHORE_POOL_SIZE,
+        CleanerChore.DEFAULT_CHORE_POOL_SIZE, "Archive");
+  }
+
+  public static DirScanPool getOldLogsScanner(Configuration conf) {

Review comment:
       Ditto.

##########
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:
       GetScanPool instead of getScanner? Scanner has special meanings in HBase which may confuse developers.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
##########
@@ -367,7 +367,8 @@
 
   private HbckChore hbckChore;
   CatalogJanitor catalogJanitorChore;
-  private DirScanPool cleanerPool;
+  private DirScanPool archiveCleanerPool;

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




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