You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@uniffle.apache.org by xi...@apache.org on 2023/02/17 09:30:54 UTC
[incubator-uniffle] branch branch-0.7 updated: [FOLLOWUP] fix: don't recreate base dir if it's already existed (#616) (#622)
This is an automated email from the ASF dual-hosted git repository.
xianjin pushed a commit to branch branch-0.7
in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git
The following commit(s) were added to refs/heads/branch-0.7 by this push:
new f7eb94ae [FOLLOWUP] fix: don't recreate base dir if it's already existed (#616) (#622)
f7eb94ae is described below
commit f7eb94ae461468ddb96279b0f634e1613aa6639a
Author: advancedxy <xi...@apache.org>
AuthorDate: Fri Feb 17 17:30:49 2023 +0800
[FOLLOWUP] fix: don't recreate base dir if it's already existed (#616) (#622)
### What changes were proposed in this pull request?
optimize the base dir init logic, it now skips recreate base dir if it's already an dir
### Why are the changes needed?
handles some corner cases such as the base dir is an mount point root path.
### Does this PR introduce _any_ user-facing change?
rss shuffle server can use mounted path as base dir directly.
### How was this patch tested?
Existing UTs.
---
.../uniffle/storage/common/LocalStorage.java | 23 ++--------
.../uniffle/storage/common/LocalStorageTest.java | 52 +++++++++++-----------
2 files changed, 31 insertions(+), 44 deletions(-)
diff --git a/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java b/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java
index 000a9aa5..1def5bfc 100644
--- a/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java
+++ b/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java
@@ -19,10 +19,8 @@ package org.apache.uniffle.storage.common;
import java.io.File;
import java.io.IOException;
-import java.nio.file.DirectoryStream;
import java.nio.file.FileStore;
import java.nio.file.Files;
-import java.nio.file.Path;
import java.util.HashSet;
import java.util.List;
import java.util.Queue;
@@ -78,14 +76,10 @@ public class LocalStorage extends AbstractStorage {
File baseFolder = new File(basePath);
try {
- if (isEmptyAndWritableDir(baseFolder)) {
- LOG.warn("Base directory is already an empty dir, skip init");
- } else {
- FileUtils.deleteDirectory(baseFolder);
- if (!baseFolder.mkdirs()) {
- throw new IOException("Failed to create base folder: " + basePath);
- }
- }
+ // similar to mkdir -p, ensure the base folder is a dir
+ FileUtils.forceMkdir(baseFolder);
+ // clean the directory if it's data left from previous ran.
+ FileUtils.cleanDirectory(baseFolder);
FileStore store = Files.getFileStore(baseFolder.toPath());
this.mountPoint = store.name();
} catch (IOException ioe) {
@@ -105,15 +99,6 @@ public class LocalStorage extends AbstractStorage {
}
}
- private boolean isEmptyAndWritableDir(File baseDir) throws IOException {
- if (baseDir.isDirectory() && baseDir.canWrite()) {
- try (DirectoryStream<Path> stream = Files.newDirectoryStream(baseDir.toPath())) {
- return !stream.iterator().hasNext();
- }
- }
- return false;
- }
-
@Override
public String getStoragePath() {
return basePath;
diff --git a/storage/src/test/java/org/apache/uniffle/storage/common/LocalStorageTest.java b/storage/src/test/java/org/apache/uniffle/storage/common/LocalStorageTest.java
index 1ed83de8..574ffc06 100644
--- a/storage/src/test/java/org/apache/uniffle/storage/common/LocalStorageTest.java
+++ b/storage/src/test/java/org/apache/uniffle/storage/common/LocalStorageTest.java
@@ -40,6 +40,7 @@ import org.apache.uniffle.storage.util.StorageType;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -67,15 +68,19 @@ public class LocalStorageTest {
testBaseDir.delete();
}
- @Test
- public void canWriteTest() {
- LocalStorage item = LocalStorage.newBuilder().basePath(testBaseDir.getAbsolutePath())
+ private LocalStorage createTestStorage(File baseDir) {
+ return LocalStorage.newBuilder().basePath(baseDir.getAbsolutePath())
.cleanupThreshold(50)
.highWaterMarkOfWrite(95)
.lowWaterMarkOfWrite(80)
.capacity(100)
.cleanIntervalMs(5000)
.build();
+ }
+
+ @Test
+ public void canWriteTest() {
+ LocalStorage item = createTestStorage(testBaseDir);
item.getMetaData().updateDiskSize(20);
assertTrue(item.canWrite());
@@ -90,7 +95,8 @@ public class LocalStorageTest {
}
@Test
- public void unRemovableEmptyDirShouldNotFail() throws IOException {
+ public void baseDirectoryInitTest() throws IOException {
+ // empty and writable base dir
File newBaseDir = new File(testBaseDirWithoutPermission, "test-new");
assertTrue(newBaseDir.mkdirs());
// then remove write permission to parent dir.
@@ -98,13 +104,21 @@ public class LocalStorageTest {
perms.add(PosixFilePermission.OWNER_READ);
perms.add(PosixFilePermission.OWNER_EXECUTE);
Files.setPosixFilePermissions(testBaseDirWithoutPermission.toPath(), perms);
- LocalStorage item = LocalStorage.newBuilder().basePath(newBaseDir.getAbsolutePath())
- .cleanupThreshold(50)
- .highWaterMarkOfWrite(95)
- .lowWaterMarkOfWrite(80)
- .capacity(100)
- .cleanIntervalMs(5000)
- .build();
+ createTestStorage(newBaseDir);
+ // non-empty and writable base dir
+ File childDir = new File(newBaseDir, "placeholder");
+ assertTrue(childDir.mkdirs());
+ createTestStorage(newBaseDir);
+ // base dir is configured to a wrong file instead of dir
+ File emptyFile = new File(newBaseDir, "empty_file");
+ assertTrue(emptyFile.createNewFile());
+ assertThrows(RuntimeException.class, () -> {
+ createTestStorage(emptyFile);
+ });
+
+ // not existed base dir should work
+ File notExisted = new File(newBaseDir, "not_existed");
+ createTestStorage(notExisted);
}
@Test
@@ -120,13 +134,7 @@ public class LocalStorageTest {
}
private LocalStorage prepareDiskItem() {
- final LocalStorage item = LocalStorage.newBuilder().basePath(testBaseDir.getAbsolutePath())
- .cleanupThreshold(50)
- .highWaterMarkOfWrite(95)
- .lowWaterMarkOfWrite(80)
- .capacity(100)
- .cleanIntervalMs(5000)
- .build();
+ final LocalStorage item = createTestStorage(testBaseDir);
RoaringBitmap partitionBitMap = RoaringBitmap.bitmapOf();
partitionBitMap.add(1);
partitionBitMap.add(2);
@@ -164,13 +172,7 @@ public class LocalStorageTest {
@Test
public void diskMetaTest() {
- LocalStorage item = LocalStorage.newBuilder().basePath(testBaseDir.getAbsolutePath())
- .cleanupThreshold(50)
- .highWaterMarkOfWrite(95)
- .lowWaterMarkOfWrite(80)
- .capacity(100)
- .cleanIntervalMs(5000)
- .build();
+ LocalStorage item = createTestStorage(testBaseDir);
List<Integer> partitionList1 = Lists.newArrayList(1, 2, 3, 4, 5);
List<Integer> partitionList2 = Lists.newArrayList(6, 7, 8, 9, 10);
List<Integer> partitionList3 = Lists.newArrayList(1, 2, 3);