This is an automated email from the ASF dual-hosted git repository. roryqi pushed a commit to branch branch-0.9 in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git
commit 9e0cf9f805462c52f60778faaa568592fac26d0b Author: Junfan Zhang <[email protected]> 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
