[GitHub] [hadoop] Hexiaoqiao commented on a diff in pull request #5744: HDFS-17046. Delete path directly when it can not be parsed in trash.

["Hexiaoqiao (via GitHub)" <gi...@apache.org>]

Hexiaoqiao commented on code in PR #5744:
URL: https://github.com/apache/hadoop/pull/5744#discussion_r1234732495


##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/TrashPolicy.java:
##########
@@ -68,10 +68,11 @@ public void initialize(Configuration conf, FileSystem fs) {
   /** 
    * Move a file or directory to the current trash directory.
    * @param path the path.
+   * @param force Whether force moving or not.
    * @return false if the item is already in the trash or trash is disabled
    * @throws IOException raised on errors performing I/O.
-   */ 
-  public abstract boolean moveToTrash(Path path) throws IOException;
+   */
+  public abstract boolean moveToTrash(Path path, boolean force) throws IOException;

Review Comment:
   Sorry I don't get meaning of the new parameter `boolean force`. 



##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestTrash.java:
##########
@@ -786,6 +793,69 @@ public void testTrashEmptier() throws Exception {
     emptierThread.join();
   }
 
+  /**
+   * Test trash emptier can whether delete non-checkpoint dir or not.
+   * @throws Exception
+   */
+  @Test
+  public void testTrashEmptierWithNonCheckpointDir() throws Exception {

Review Comment:
   This new unit test seems not cover the case you try to fix, right? It could pass with/without this improvement.



##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/TrashPolicyDefault.java:
##########
@@ -374,8 +382,14 @@ private void deleteCheckpoint(Path trashRoot, boolean deleteImmediately)
       try {
         time = getTimeFromCheckpoint(name);
       } catch (ParseException e) {
-        LOG.warn("Unexpected item in trash: "+dir+". Ignoring.");
-        continue;
+        if (cleanNonCheckpointUnderTrashRoot) {
+          this.moveToTrash(path, true);

Review Comment:
   Why move to trash again since it has been already under Trash home AND has configed to clean? IMO, it is safe to delete them directly.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org