HIVE-29117: Refactor deleteDir and moveToTrash

Author: wecharyu

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

@@ -33,11 +33,10 @@ public interface MetaStoreFS {
    *
    * @param f
    * @param ifPurge
-   * @param recursive
    * @return true on success
    * @throws MetaException
    */
-  public boolean deleteDir(FileSystem fs, Path f, boolean recursive,
+  public boolean deleteDir(FileSystem fs, Path f,
       boolean ifPurge, Configuration conf) throws MetaException;
 
 }

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

@@ -472,7 +472,7 @@ 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);
+    return fsHandler.deleteDir(fs, f, ifPurge, conf);
   }
 
   public void recycleDirToCmPath(Path f, boolean ifPurge) throws MetaException {

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

@@ -35,12 +35,9 @@
 import org.apache.hadoop.fs.PathFilter;
 import org.apache.hadoop.fs.RemoteIterator;
 import org.apache.hadoop.fs.Trash;
-import org.apache.hadoop.hdfs.DistributedFileSystem;
-import org.apache.hadoop.hdfs.protocol.SnapshotException;
 import org.apache.hadoop.hive.metastore.api.MetaException;
 import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
 import org.apache.hadoop.hive.metastore.conf.MetastoreConf.ConfVars;
-import org.apache.hadoop.ipc.RemoteException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -77,72 +74,19 @@ public boolean accept(Path p) {
    * @param f path of file or directory to move to trash.
    * @param conf configuration object
    * @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;
+  public static boolean moveToTrash(FileSystem fs, Path f, Configuration conf) {
+    LOG.debug("moving  " + f + " to trash");
     try {
-      if (!fs.exists(f)) {
-        LOG.warn("The path to moveToTrash does not exist: " + f);
+      boolean result = Trash.moveToAppropriateTrash(fs, f, conf);
+      if (result) {
+        LOG.trace("Moved to trash: " + f);
         return true;
       }
-      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.");
-    }
-
-    try {
-      result = fs.delete(f, true);
-
-    } catch (RemoteException | SnapshotException se) {
-      // If this is snapshot exception or the cause is snapshot replication from HDFS, could be the case where the
-      // snapshots were created by replication, so in that case attempt to delete the replication related snapshots,
-      // if the exists and then re attempt delete.
-      if (se instanceof SnapshotException || se.getCause() instanceof SnapshotException || se.getMessage()
-          .contains("Snapshot"))
-        deleteReplRelatedSnapshots(fs, f);
-      // retry delete after attempting to delete replication related snapshots
-      result = fs.delete(f, true);
-    }
-    if (!result) {
-      LOG.error("Failed to delete " + f);
-    }
-    return result;
-  }
-
-  /**
-   * Attempts to delete the replication related snapshots
-   * @param fs the filesystem
-   * @param path path where the snapshots are supposed to exists.
-   */
-  private static void deleteReplRelatedSnapshots(FileSystem fs, Path path) {
-    try {
-      DistributedFileSystem dfs = (DistributedFileSystem) fs;
-      // List the snapshot directory.
-      FileStatus[] listing = fs.listStatus(new Path(path, ".snapshot"));
-      for (FileStatus elem : listing) {
-        // if the snapshot name has replication related suffix, then delete that snapshot.
-        if (elem.getPath().getName().endsWith("replOld") || elem.getPath().getName().endsWith("replNew")) {
-          dfs.deleteSnapshot(path, elem.getPath().getName());
-        }
-      }
-    } catch (Exception ioe) {
-      // Ignore since this method is used as part of purge which actually ignores all exception, if the directory can
-      // not be deleted, so preserve the same behaviour.
-      LOG.warn("Couldn't clean up replication related snapshots", ioe);
+      LOG.warn("Failed to move path to trash: " + f, ioe);
     }
+    return false;
   }
 
   /**

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreFsImpl.java

@@ -20,11 +20,15 @@
 
 import org.apache.hadoop.hive.metastore.utils.FileUtils;
 import org.apache.hadoop.hive.metastore.utils.MetaStoreUtils;
+import org.apache.hadoop.ipc.RemoteException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hdfs.DistributedFileSystem;
+import org.apache.hadoop.hdfs.protocol.SnapshotException;
 import org.apache.hadoop.hive.metastore.api.MetaException;
 
 public class HiveMetaStoreFsImpl implements MetaStoreFS {
@@ -33,15 +37,56 @@ public class HiveMetaStoreFsImpl implements MetaStoreFS {
       .getLogger("hive.metastore.hivemetastoreFsimpl");
 
   @Override
-  public boolean deleteDir(FileSystem fs, Path f, boolean recursive,
-      boolean ifPurge, Configuration conf) throws MetaException {
+  public boolean deleteDir(FileSystem fs, Path f, boolean ifPurge, Configuration conf)
+      throws MetaException {
     try {
-      if (FileUtils.moveToTrash(fs, f, conf, ifPurge)) {
+      if (!fs.exists(f)) {
+        LOG.warn("The path to delete does not exist: " + f);
         return true;
       }
+      if (!ifPurge && FileUtils.moveToTrash(fs, f, conf)) {
+        return true;
+      }
+      try {
+        // for whatever failure reason including that trash has lower encryption zone
+        // retry with force delete
+        fs.delete(f, true);
+      } catch (RemoteException | SnapshotException se) {
+        // If this is snapshot exception or the cause is snapshot replication from HDFS, could be the case where the
+        // snapshots were created by replication, so in that case attempt to delete the replication related snapshots,
+        // if the exists and then re attempt delete.
+        if (se instanceof SnapshotException || se.getCause() instanceof SnapshotException || se.getMessage()
+            .contains("Snapshot"))
+          deleteReplRelatedSnapshots(fs, f);
+        // retry delete after attempting to delete replication related snapshots
+        fs.delete(f, true);
+      }
     } catch (Exception e) {
       MetaStoreUtils.throwMetaException(e);
     }
-    throw new MetaException("Unable to delete directory: " + f);
+    return true;
+  }
+
+  /**
+   * Attempts to delete the replication related snapshots
+   * @param fs the filesystem
+   * @param path path where the snapshots are supposed to exists.
+   */
+  private static void deleteReplRelatedSnapshots(FileSystem fs, Path path) {
+    try {
+      DistributedFileSystem dfs = (DistributedFileSystem) fs;
+      // List the snapshot directory.
+      FileStatus[] listing = fs.listStatus(new Path(path, ".snapshot"));
+      for (FileStatus elem : listing) {
+        // if the snapshot name has replication related suffix, then delete that snapshot.
+        if (elem.getPath().getName().endsWith("replOld") || elem.getPath().getName().endsWith("replNew")) {
+          dfs.deleteSnapshot(path, elem.getPath().getName());
+        }
+      }
+    } catch (Exception ioe) {
+      // Ignore since this method is used as part of purge which actually ignores all exception, if the directory can
+      // not be deleted, so preserve the same behaviour.
+      LOG.warn("Couldn't clean up replication related snapshots", ioe);
+    }
   }
 }