Skip to content

Commit

Permalink
[#1634] fix(server): remove app folder if app is expired (#1635)
Browse files Browse the repository at this point in the history
### 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.
  • Loading branch information
zuston committed Apr 13, 2024
1 parent 07d8cb5 commit 6ea4300
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 6ea4300

Please sign in to comment.