From 6ea43002dc005f5127cccf45fa953769628a2965 Mon Sep 17 00:00:00 2001
From: Junfan Zhang <zuston@apache.org>
Date: Sat, 13 Apr 2024 20:31:25 +0800
Subject: [PATCH] [#1634] fix(server): remove app folder if app is expired
 (#1635)

### What changes were proposed in this pull request?

Always executing the removeResources no matter whether blockIds remain.

### Why are the changes needed?

Fix: #1634

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Using existing unit tests, but additionally checking the app folder after app is expired.
---
 .../apache/uniffle/server/ShuffleTaskManager.java    |  9 +--------
 .../uniffle/server/ShuffleTaskManagerTest.java       | 12 ++++++++++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java b/server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java
index 9f82870dae..95b70031fb 100644
--- a/server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java
+++ b/server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java
@@ -18,7 +18,6 @@
 package org.apache.uniffle.server;
 
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
@@ -763,22 +762,16 @@ public void removeResources(String appId, boolean checkAppExpired) {
         return;
       }
       final long start = System.currentTimeMillis();
-      String user = getUserByAppId(appId);
       ShuffleTaskInfo shuffleTaskInfo = shuffleTaskInfos.remove(appId);
       if (shuffleTaskInfo == null) {
         LOG.info("Resource for appId[" + appId + "] had been removed before.");
         return;
       }
 
-      final Map<Integer, Roaring64NavigableMap> shuffleToCachedBlockIds =
-          shuffleTaskInfo.getCachedBlockIds();
       partitionsToBlockIds.remove(appId);
       shuffleBufferManager.removeBuffer(appId);
       shuffleFlushManager.removeResources(appId);
-      if (!shuffleToCachedBlockIds.isEmpty()) {
-        storageManager.removeResources(
-            new AppPurgeEvent(appId, user, new ArrayList<>(shuffleToCachedBlockIds.keySet())));
-      }
+      storageManager.removeResources(new AppPurgeEvent(appId, shuffleTaskInfo.getUser()));
       if (shuffleTaskInfo.hasHugePartition()) {
         ShuffleServerMetrics.gaugeAppWithHugePartitionNum.dec();
         ShuffleServerMetrics.gaugeHugePartitionNum.dec(shuffleTaskInfo.getHugePartitionSize());
diff --git a/server/src/test/java/org/apache/uniffle/server/ShuffleTaskManagerTest.java b/server/src/test/java/org/apache/uniffle/server/ShuffleTaskManagerTest.java
index 70d2529476..67410b3c05 100644
--- a/server/src/test/java/org/apache/uniffle/server/ShuffleTaskManagerTest.java
+++ b/server/src/test/java/org/apache/uniffle/server/ShuffleTaskManagerTest.java
@@ -470,7 +470,11 @@ public void removeShuffleDataWithHdfsTest() throws Exception {
     assertTrue(fs.exists(new Path(appBasePath)));
     assertNull(shuffleBufferManager.getBufferPool().get(appId).get(0));
     assertNotNull(shuffleBufferManager.getBufferPool().get(appId).get(1));
+
+    // the shufflePurgeEvent only will delete the children folders
+    // Once the app is expired, all the app folders should be deleted.
     shuffleTaskManager.removeResources(appId, false);
+    assertFalse(fs.exists(new Path(appBasePath)));
   }
 
   @Test
@@ -528,6 +532,14 @@ public void removeShuffleDataWithLocalfileTest() throws Exception {
         assertEquals(0, files.length);
       }
     }
+
+    // the shufflePurgeEvent only will delete the children folders
+    // Once the app is expired, all the app folders should be deleted.
+    shuffleTaskManager.removeResources(appId, false);
+    for (String path : conf.get(ShuffleServerConf.RSS_STORAGE_BASE_PATH)) {
+      String appPath = path + "/" + appId;
+      assertFalse(new File(appPath).exists());
+    }
   }
 
   @Test