HIVE-29117: Refactor deleteDir and moveToTrash (#6006)

* HIVE-29117: Refactor deleteDir and moveToTrash

* fix compile and remove unnecessary MetaStoreFS interface

* return void in deleteDir()

* fix sonar reports warnnings

* Revert "return void in deleteDir()"

This reverts commit 7ad708b.

Author: wecharyu

common/src/java/org/apache/hadoop/hive/common/FileUtils.java

@@ -57,7 +57,6 @@
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.PathFilter;
 import org.apache.hadoop.fs.RemoteIterator;
-import org.apache.hadoop.fs.Trash;
 import org.apache.hadoop.fs.permission.FsAction;
 import org.apache.hadoop.fs.FSDataOutputStream;
 import org.apache.hadoop.fs.PathExistsException;
@@ -963,41 +962,6 @@ public static boolean distCpWithSnapshot(String oldSnapshot, String newSnapshot,
     return copied;
   }
 
-  /**
-   * Move a particular file or directory to the trash.
-   * @param fs FileSystem to use
-   * @param f path of file or directory to move to trash.
-   * @param conf
-   * @return true if move successful
-   * @throws IOException
-   */
-  public static boolean moveToTrash(FileSystem fs, Path f, Configuration conf, boolean purge)
-      throws IOException {
-    LOG.debug("deleting  " + f);
-    boolean result = false;
-    try {
-      if(purge) {
-        LOG.debug("purge is set to true. Not moving to Trash " + f);
-      } else {
-        result = Trash.moveToAppropriateTrash(fs, f, conf);
-        if (result) {
-          LOG.trace("Moved to trash: " + f);
-          return true;
-        }
-      }
-    } catch (IOException ioe) {
-      // for whatever failure reason including that trash has lower encryption zone
-      // retry with force delete
-      LOG.warn(ioe.getMessage() + "; Force to delete it.");
-    }
-
-    result = fs.delete(f, true);
-    if (!result) {
-      LOG.error("Failed to delete " + f);
-    }
-    return result;
-  }
-
   public static boolean rename(FileSystem fs, Path sourcePath,
                                Path destPath, Configuration conf) throws IOException {
     LOG.info("Renaming " + sourcePath + " to " + destPath);

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

@@ -1736,11 +1736,6 @@ public static enum ConfVars {
     CLI_PROMPT("hive.cli.prompt", "hive",
         "Command line prompt configuration value. Other hiveconf can be used in this configuration value. \n" +
         "Variable substitution will only be invoked at the Hive CLI startup."),
-    /**
-     * @deprecated Use MetastoreConf.FS_HANDLER_CLS
-     */
-    @Deprecated
-    HIVE_METASTORE_FS_HANDLER_CLS("hive.metastore.fs.handler.class", "org.apache.hadoop.hive.metastore.HiveMetaStoreFsImpl", ""),
 
     // Things we log in the jobconf
 

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/archive/AlterTableArchiveUtils.java

@@ -97,7 +97,7 @@ static Path getInterMediateDir(Path dir, Configuration conf, ConfVars suffixConf
   static void deleteDir(Path dir, boolean shouldEnableCm, Configuration conf) throws HiveException {
     try {
       Warehouse wh = new Warehouse(conf);
-      wh.deleteDir(dir, true, false, shouldEnableCm);
+      wh.deleteDir(dir, false, shouldEnableCm);
     } catch (MetaException e) {
       throw new HiveException(e);
     }

ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java

@@ -5948,13 +5948,13 @@ public static boolean trashFiles(final FileSystem fs, final FileStatus[] statuse
     final SessionState parentSession = SessionState.get();
     for (final FileStatus status : statuses) {
       if (null == pool) {
-        result &= FileUtils.moveToTrash(fs, status.getPath(), conf, purge);
+        result &= org.apache.hadoop.hive.metastore.utils.FileUtils.deleteDir(fs, status.getPath(), purge, conf);
       } else {
         futures.add(pool.submit(new Callable<Boolean>() {
           @Override
           public Boolean call() throws Exception {
             SessionState.setCurrentSessionState(parentSession);
-            return FileUtils.moveToTrash(fs, status.getPath(), conf, purge);
+            return org.apache.hadoop.hive.metastore.utils.FileUtils.deleteDir(fs, status.getPath(), purge, conf);
           }
         }));
       }

ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java

@@ -1062,7 +1062,7 @@ private void truncateTempTable(Table table) throws TException {
         HdfsUtils.HadoopFileStatus status = HdfsUtils.HadoopFileStatus.createInstance(conf, fs, location);
         FileStatus targetStatus = fs.getFileStatus(location);
         String targetGroup = targetStatus == null ? null : targetStatus.getGroup();
-        FileUtils.moveToTrash(fs, location, conf, isSkipTrash);
+        org.apache.hadoop.hive.metastore.utils.FileUtils.deleteDir(fs, location, isSkipTrash, conf);
         fs.mkdirs(location);
         HdfsUtils.setFullFileStatus(conf, status, targetGroup, fs, location, false);
       } else {
@@ -1124,7 +1124,7 @@ private void dropTempTable(Table table, boolean deleteData,
     // Delete table data
     if (deleteData && !isExternalTable(table)) {
       try {
-        getWh().deleteDir(tablePath, true, ifPurge, false);
+        getWh().deleteDir(tablePath, ifPurge, false);
       } catch (Exception err) {
         LOG.error("Failed to delete temp table directory: " + tablePath, err);
         // Forgive error
@@ -2215,7 +2215,7 @@ private boolean deletePartitionLocation(Partition partition, boolean purgeData)
       Path path = getWh().getDnsPath(new Path(location));
       try {
         do {
-          if (!getWh().deleteDir(path, true, purgeData, false)) {
+          if (!getWh().deleteDir(path, purgeData, false)) {
             throw new MetaException("Unable to delete partition at " + location);
           }
           path = path.getParent();

ql/src/java/org/apache/hadoop/hive/ql/parse/MetaDataExportListener.java

@@ -90,7 +90,7 @@ private void export_meta_data(PreDropTableEvent tableEvent) throws MetaException
       EximUtil.createExportDump(fs, outFile, mTbl, null, null,
           new HiveConf(conf, MetaDataExportListener.class));
       if (moveMetadataToTrash == true) {
-        wh.deleteDir(metaPath, true, false, false);
+        wh.deleteDir(metaPath, false, false);
       }
     } catch (IOException | SemanticException e) {
       throw new MetaException(e.getMessage());

ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/FSRemover.java

@@ -122,7 +122,7 @@ private List<Path> removeFiles(CleanupRequest cr)
       if (needCmRecycle) {
         replChangeManager.recycle(dead, ReplChangeManager.RecycleType.MOVE, cr.isPurge());
       }
-      if (FileUtils.moveToTrash(fs, dead, conf, cr.isPurge())) {
+      if (FileUtils.deleteDir(fs, dead, cr.isPurge(), conf)) {
         deleted.add(dead);
       }
     }

ql/src/test/org/apache/hadoop/hive/ql/metadata/TestSessionHiveMetastoreClientExchangePartitionsTempTable.java

@@ -356,7 +356,7 @@ private Table createNonTempTable(String dbName, String tableName, List<FieldSche
   }
 
   private void cleanTempTableDir(Table table) throws MetaException {
-    wh.deleteDir(new Path(table.getSd().getLocation()), true, false, false);
+    wh.deleteDir(new Path(table.getSd().getLocation()), false, false);
   }
 
 }

standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreFS.java

@@ -40,7 +40,6 @@
 import org.apache.hadoop.hive.metastore.conf.MetastoreConf.ConfVars;
 import org.apache.hadoop.hive.metastore.utils.FileUtils;
 import org.apache.hadoop.hive.metastore.utils.HdfsUtils;
-import org.apache.hadoop.hive.metastore.utils.JavaUtils;
 import org.apache.hadoop.hive.metastore.utils.MetaStoreUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -57,7 +56,6 @@
 import org.apache.hadoop.hive.metastore.api.Partition;
 import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
 import org.apache.hadoop.hive.metastore.api.Table;
-import org.apache.hadoop.util.ReflectionUtils;
 
 import static org.apache.hadoop.hive.common.AcidConstants.SOFT_DELETE_TABLE_PATTERN;
 import static org.apache.hadoop.hive.common.AcidConstants.SOFT_DELETE_PATH_SUFFIX;
@@ -83,7 +81,6 @@ public class Warehouse {
 
   public static final Logger LOG = LoggerFactory.getLogger("hive.metastore.warehouse");
 
-  private MetaStoreFS fsHandler = null;
   private boolean storageAuthCheck = false;
   private ReplChangeManager cm = null;
 
@@ -95,27 +92,11 @@ public Warehouse(Configuration conf) throws MetaException {
           + " is not set in the config or blank");
     }
     whRootExternalString = MetastoreConf.getVar(conf, ConfVars.WAREHOUSE_EXTERNAL);
-    fsHandler = getMetaStoreFsHandler(conf);
     cm = ReplChangeManager.getInstance(conf);
     storageAuthCheck = MetastoreConf.getBoolVar(conf, ConfVars.AUTHORIZATION_STORAGE_AUTH_CHECKS);
     isTenantBasedStorage = MetastoreConf.getBoolVar(conf, ConfVars.ALLOW_TENANT_BASED_STORAGE);
   }
 
-  private MetaStoreFS getMetaStoreFsHandler(Configuration conf)
-      throws MetaException {
-    String handlerClassStr = MetastoreConf.getVar(conf, ConfVars.FS_HANDLER_CLS);
-    try {
-      Class<? extends MetaStoreFS> handlerClass = (Class<? extends MetaStoreFS>) Class
-          .forName(handlerClassStr, true, JavaUtils.getClassLoader());
-      MetaStoreFS handler = ReflectionUtils.newInstance(handlerClass, conf);
-      return handler;
-    } catch (ClassNotFoundException e) {
-      throw new MetaException("Error in loading MetaStoreFS handler."
-          + e.getMessage());
-    }
-  }
-
-
   /**
    * Helper functions to convert IOException to MetaException
    */
@@ -451,15 +432,15 @@ void addToChangeManagement(Path file) throws MetaException {
     }
   }
 
-  public boolean deleteDir(Path f, boolean recursive, Database db) throws MetaException {
-    return deleteDir(f, recursive, false, db);
+  public boolean deleteDir(Path f, Database db) throws MetaException {
+    return deleteDir(f, false, db);
   }
 
-  public boolean deleteDir(Path f, boolean recursive, boolean ifPurge, Database db) throws MetaException {
-    return deleteDir(f, recursive, ifPurge, ReplChangeManager.isSourceOfReplication(db));
+  public boolean deleteDir(Path f, boolean ifPurge, Database db) throws MetaException {
+    return deleteDir(f, ifPurge, ReplChangeManager.isSourceOfReplication(db));
   }
 
-  public boolean deleteDir(Path f, boolean recursive, boolean ifPurge, boolean needCmRecycle) throws MetaException {
+  public boolean deleteDir(Path f, boolean ifPurge, boolean needCmRecycle) throws MetaException {
     if (needCmRecycle) {
       try {
         cm.recycle(f, RecycleType.MOVE, ifPurge);
@@ -472,7 +453,13 @@ public boolean deleteDir(Path f, boolean recursive, boolean ifPurge, boolean nee
       LOG.warn("Har path {} is not supported to delete, skipping it.", f);
       return true;
     }
-    return fsHandler.deleteDir(fs, f, recursive, ifPurge, conf);
+    boolean delete = false;
+    try {
+      delete = FileUtils.deleteDir(fs, f, ifPurge, conf);
+    } catch (IOException e) {
+      MetaStoreUtils.throwMetaException(e);
+    }
+    return delete;
   }
 
   public void recycleDirToCmPath(Path f, boolean ifPurge) throws MetaException {