(incubator-uniffle) branch master updated: [#1634] fix(server): remove app folder if app is expired (#1635)

[roryqi]

This is an automated email from the ASF dual-hosted git repository.

roryqi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git


The following commit(s) were added to refs/heads/master by this push:
     new 6ea43002d [#1634] fix(server): remove app folder if app is expired 
(#1635)
6ea43002d is described below

commit 6ea43002dc005f5127cccf45fa953769628a2965
Author: Junfan Zhang <zus...@apache.org>
AuthorDate: Sat Apr 13 20:31:25 2024 +0800

    [#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.
---
 .../java/org/apache/uniffle/server/ShuffleTaskManager.java   |  9 +--------
 .../org/apache/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 9f82870da..95b70031f 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 class ShuffleTaskManager {
         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 70d252947..67410b3c0 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 class ShuffleTaskManagerTest extends HadoopTestBase 
{
     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 class ShuffleTaskManagerTest extends HadoopTestBase 
{
         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