[GitHub] [hbase] busbey commented on a change in pull request #2038: HBASE-24687 MobFileCleanerChore uses a new Connection for each table …

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

busbey commented on a change in pull request #2038:
URL: https://github.com/apache/hbase/pull/2038#discussion_r457824834



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/mob/MobFileCleanerChore.java
##########
@@ -79,7 +76,12 @@ public MobFileCleanerChore(HMaster master) {
           MobConstants.DEFAULT_MOB_CLEANER_PERIOD),
         TimeUnit.SECONDS);
     this.master = master;
-    cleaner = new ExpiredMobFileCleaner();
+    try (Connection conn = ConnectionFactory.createConnection(master.getConfiguration())) {

Review comment:
       this will call `conn.close` at the end of the try block.
   
   Can we use `master.getConnection()` and just make use of the shared connection rather than making a new one?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/mob/MobFileCleanerChore.java
##########
@@ -156,140 +160,139 @@ public void cleanupObsoleteMobFiles(Configuration conf, TableName table) throws
     // So, if MOB file creation time is greater than this maxTimeToArchive,
     // this will be skipped and won't be archived.
     long maxCreationTimeToArchive = EnvironmentEdgeManager.currentTime() - minAgeToArchive;
-    try (final Connection conn = ConnectionFactory.createConnection(conf);
-        final Admin admin = conn.getAdmin();) {
-      TableDescriptor htd = admin.getDescriptor(table);
-      List<ColumnFamilyDescriptor> list = MobUtils.getMobColumnFamilies(htd);
-      if (list.size() == 0) {
-        LOG.info("Skipping non-MOB table [{}]", table);
-        return;
-      } else {
-        LOG.info("Only MOB files whose creation time older than {} will be archived, table={}",
-          maxCreationTimeToArchive, table);
-      }
 
-      Path rootDir = CommonFSUtils.getRootDir(conf);
-      Path tableDir = CommonFSUtils.getTableDir(rootDir, table);
-      // How safe is this call?
-      List<Path> regionDirs = FSUtils.getRegionDirs(FileSystem.get(conf), tableDir);
+    Admin admin = connection.getAdmin();

Review comment:
       this should be in a try-with-resources to ensure we close the `Admin` instance.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/mob/MobFileCleanerChore.java
##########
@@ -65,11 +66,7 @@
   private static final Logger LOG = LoggerFactory.getLogger(MobFileCleanerChore.class);
   private final HMaster master;
   private ExpiredMobFileCleaner cleaner;
-
-  static {
-    Configuration.addDeprecation(MobConstants.DEPRECATED_MOB_CLEANER_PERIOD,

Review comment:
       we should leave this deprecation in. it hasn't been in a release yet.




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

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