From 5d6280b097f0dd2334081ded848a035b10783d96 Mon Sep 17 00:00:00 2001 From: Xing Lin Date: Tue, 6 Sep 2022 10:16:32 -0700 Subject: [PATCH 1/4] HADOOP-18444 Add Support for localized trash for ViewFileSystem in Trash.moveToAppropriateTrash Signed-off-by: Xing Lin --- .../main/java/org/apache/hadoop/fs/Trash.java | 23 ++++++++++ .../hadoop/fs/viewfs/TestViewFsTrash.java | 44 ++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java index a58a1a3cb8eb1..23f5d8ef0de03 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java @@ -23,8 +23,10 @@ import org.apache.hadoop.classification.InterfaceStability; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.conf.Configured; +import org.apache.hadoop.fs.viewfs.ViewFileSystem; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import static org.apache.hadoop.fs.viewfs.Constants.*; /** * Provides a trash facility which supports pluggable Trash policies. @@ -94,6 +96,27 @@ public static boolean moveToAppropriateTrash(FileSystem fs, Path p, LOG.warn("Failed to get server trash configuration", e); throw new IOException("Failed to get server trash configuration", e); } + + // Directly use viewfs if localized trash is enabled + if (fs instanceof ViewFileSystem && + conf.getBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, + CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT_DEFAULT)) { + // Save the original config in savedValue for localized trash config. + String savedValue = fs.getConf().get(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT); + fs.getConf().setBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, true); + Trash trash = new Trash(fs, conf); + boolean res = trash.moveToTrash(p); + + // Restore the original value of localized trash config + if (savedValue != null) { + fs.getConf().set(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, savedValue); + } else { + fs.getConf().unset(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT); + } + + return res; + } + Trash trash = new Trash(fullyResolvedFs, conf); return trash.moveToTrash(fullyResolvedPath); } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java index 8e5fa72f7ed71..b2e5c887013b1 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java @@ -17,15 +17,23 @@ */ package org.apache.hadoop.fs.viewfs; +import java.io.DataOutputStream; +import java.io.IOException; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.FileSystemTestHelper; import org.apache.hadoop.fs.FsConstants; +import org.apache.hadoop.fs.LocalFileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.TestTrash; +import org.apache.hadoop.fs.Trash; +import org.apache.hadoop.fs.TrashPolicyDefault; import org.junit.After; import org.junit.Before; import org.junit.Test; +import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.*; +import static org.apache.hadoop.fs.viewfs.Constants.*; +import static org.junit.Assert.*; public class TestViewFsTrash { FileSystem fsTarget; // the target file system - the mount will point here @@ -65,5 +73,39 @@ public void testTrash() throws Exception { TestTrash.trashShell(conf, fileSystemTestHelper.getTestRootPath(fsView), fsView, new Path(fileSystemTestHelper.getTestRootPath(fsView), ".Trash/Current")); } - + + @Test + public void testLocalizedTrashInMoveToAppropriateTrash() throws IOException { + Configuration conf2 = new Configuration(conf); + + // Enable moveToTrash and add a mount point for /data + conf2.setLong(FS_TRASH_INTERVAL_KEY, 1); + ConfigUtil.addLink(conf2, "/data", new Path(fileSystemTestHelper.getAbsoluteTestRootPath(fsTarget), "data").toUri()); + + // Default case. file should be moved to fsTarget.getTrashRoot()/resolvedPath + conf2.setBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, false); + FileSystem fsView2 = FileSystem.get(conf2); + Path f = new Path("/data/testfile.txt"); + DataOutputStream out = fsView2.create(f); + out.writeBytes("testfile.txt:" + f); + out.close(); + Path resolvedFile = fsView2.resolvePath(f); + + Trash.moveToAppropriateTrash(fsView2, f, conf2); + Trash trash = new Trash(fsTarget, conf2); + Path movedPath = Path.mergePaths(trash.getCurrentTrashDir(f), resolvedFile); + assertTrue("File not in trash", fsTarget.exists(movedPath)); + + // Turn on localized trash. File should be moved to viewfs:/data/.Trash/{user}/Current. + conf2.setBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, true); + fsView2 = FileSystem.get(conf2); + out = fsView2.create(f); + out.writeBytes("testfile.txt:" + f); + out.close(); + + Trash.moveToAppropriateTrash(fsView2, f, conf2); + trash = new Trash(fsView2, conf2); + movedPath = Path.mergePaths(trash.getCurrentTrashDir(f), f); + assertTrue("File not in localized trash", fsView2.exists(movedPath)); + } } From 3d92861ac0967cb9341897fc022e5c6fc2aa6d79 Mon Sep 17 00:00:00 2001 From: Xing Lin Date: Sun, 11 Sep 2022 17:37:02 -0700 Subject: [PATCH 2/4] Improved tests with suggestions from Steve Loughran --- .../main/java/org/apache/hadoop/fs/Trash.java | 13 +++++-- .../hadoop/fs/viewfs/TestViewFsTrash.java | 35 +++++++++---------- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java index 23f5d8ef0de03..a8dc28e895365 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java @@ -97,9 +97,16 @@ public static boolean moveToAppropriateTrash(FileSystem fs, Path p, throw new IOException("Failed to get server trash configuration", e); } - // Directly use viewfs if localized trash is enabled - if (fs instanceof ViewFileSystem && - conf.getBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, + /* + * In HADOOP-18144, we fixed the logical path vs. target path bug of getTrashRoot() in ViewFileSystem. + * moveToTrash works for ViewFileSystem now. ViewFileSystem will do path resolution internally by itself. + * + * When localized trash flag is enabled: + * 1). if fs is a ViewFileSystem, we can initialize Trash() with this ViewFileSystem object; + * 2). When fs is not a ViewFileSystem, the only place we would need to resolve a path is for symbolic links. + * However, symlink is not enabled in Hadoop due to the complexity to support it (HADOOP-10019). + */ + if (conf.getBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT_DEFAULT)) { // Save the original config in savedValue for localized trash config. String savedValue = fs.getConf().get(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT); diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java index b2e5c887013b1..06cbdab8d210f 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java @@ -28,6 +28,7 @@ import org.apache.hadoop.fs.TestTrash; import org.apache.hadoop.fs.Trash; import org.apache.hadoop.fs.TrashPolicyDefault; +import org.apache.hadoop.fs.contract.ContractTestUtils; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -77,6 +78,7 @@ public void testTrash() throws Exception { @Test public void testLocalizedTrashInMoveToAppropriateTrash() throws IOException { Configuration conf2 = new Configuration(conf); + Path testFile = new Path("/data/testfile.txt"); // Enable moveToTrash and add a mount point for /data conf2.setLong(FS_TRASH_INTERVAL_KEY, 1); @@ -84,28 +86,25 @@ public void testLocalizedTrashInMoveToAppropriateTrash() throws IOException { // Default case. file should be moved to fsTarget.getTrashRoot()/resolvedPath conf2.setBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, false); - FileSystem fsView2 = FileSystem.get(conf2); - Path f = new Path("/data/testfile.txt"); - DataOutputStream out = fsView2.create(f); - out.writeBytes("testfile.txt:" + f); - out.close(); - Path resolvedFile = fsView2.resolvePath(f); + try (FileSystem fsView2 = FileSystem.get(conf2)) { + FileSystemTestHelper.createFile(fsView2, testFile); + Path resolvedFile = fsView2.resolvePath(testFile); - Trash.moveToAppropriateTrash(fsView2, f, conf2); - Trash trash = new Trash(fsTarget, conf2); - Path movedPath = Path.mergePaths(trash.getCurrentTrashDir(f), resolvedFile); - assertTrue("File not in trash", fsTarget.exists(movedPath)); + Trash.moveToAppropriateTrash(fsView2, testFile, conf2); + Trash trash = new Trash(fsTarget, conf2); + Path movedPath = Path.mergePaths(trash.getCurrentTrashDir(testFile), resolvedFile); + ContractTestUtils.assertPathExists(fsTarget, "File not in trash", movedPath); + } // Turn on localized trash. File should be moved to viewfs:/data/.Trash/{user}/Current. conf2.setBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, true); - fsView2 = FileSystem.get(conf2); - out = fsView2.create(f); - out.writeBytes("testfile.txt:" + f); - out.close(); + try (FileSystem fsView2 = FileSystem.get(conf2)) { + FileSystemTestHelper.createFile(fsView2, testFile); - Trash.moveToAppropriateTrash(fsView2, f, conf2); - trash = new Trash(fsView2, conf2); - movedPath = Path.mergePaths(trash.getCurrentTrashDir(f), f); - assertTrue("File not in localized trash", fsView2.exists(movedPath)); + Trash.moveToAppropriateTrash(fsView2, testFile, conf2); + Trash trash = new Trash(fsView2, conf2); + Path movedPath = Path.mergePaths(trash.getCurrentTrashDir(testFile), testFile); + ContractTestUtils.assertPathExists(fsView2, "File not in localized trash", movedPath); + } } } From 6c222a4efd891e687127f564792e8de26ee0949b Mon Sep 17 00:00:00 2001 From: Xing Lin Date: Tue, 20 Sep 2022 20:40:25 -0700 Subject: [PATCH 3/4] Removed save and restore of localized trash flag in fs.Conf() --- .../main/java/org/apache/hadoop/fs/Trash.java | 30 ++++++++----------- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java index a8dc28e895365..3025b5ab996d7 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java @@ -98,30 +98,24 @@ public static boolean moveToAppropriateTrash(FileSystem fs, Path p, } /* - * In HADOOP-18144, we fixed the logical path vs. target path bug of getTrashRoot() in ViewFileSystem. - * moveToTrash works for ViewFileSystem now. ViewFileSystem will do path resolution internally by itself. + * In HADOOP-18144, we changed getTrashRoot() in ViewFileSystem to return a + * viewFS path, instead of a targetFS path. moveToTrash works for + * ViewFileSystem now. ViewFileSystem will do path resolution internally by + * itself. * * When localized trash flag is enabled: - * 1). if fs is a ViewFileSystem, we can initialize Trash() with this ViewFileSystem object; - * 2). When fs is not a ViewFileSystem, the only place we would need to resolve a path is for symbolic links. - * However, symlink is not enabled in Hadoop due to the complexity to support it (HADOOP-10019). + * 1). if fs is a ViewFileSystem, we can initialize Trash() with a + * ViewFileSystem object; + * 2). When fs is not a ViewFileSystem, the only place we would need to + * resolve a path is for symbolic links. However, symlink is not + * enabled in Hadoop due to the complexity to support it + * (HADOOP-10019). */ if (conf.getBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT_DEFAULT)) { - // Save the original config in savedValue for localized trash config. - String savedValue = fs.getConf().get(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT); - fs.getConf().setBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, true); + //TODO: Create a new FS object, to ensure the latest conf is used. Trash trash = new Trash(fs, conf); - boolean res = trash.moveToTrash(p); - - // Restore the original value of localized trash config - if (savedValue != null) { - fs.getConf().set(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, savedValue); - } else { - fs.getConf().unset(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT); - } - - return res; + return trash.moveToTrash(p); } Trash trash = new Trash(fullyResolvedFs, conf); From fe212ca77405792f3023dfc1330768e71316876c Mon Sep 17 00:00:00 2001 From: Xing Lin Date: Wed, 21 Sep 2022 21:03:17 -0700 Subject: [PATCH 4/4] Removed a comment. --- .../hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java index 3025b5ab996d7..5c5fa0237ea66 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java @@ -113,7 +113,6 @@ public static boolean moveToAppropriateTrash(FileSystem fs, Path p, */ if (conf.getBoolean(CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT, CONFIG_VIEWFS_TRASH_FORCE_INSIDE_MOUNT_POINT_DEFAULT)) { - //TODO: Create a new FS object, to ensure the latest conf is used. Trash trash = new Trash(fs, conf); return trash.moveToTrash(p); }