[
https://issues.apache.org/jira/browse/HADOOP-18444?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17602495#comment-17602495
]
ASF GitHub Bot commented on HADOOP-18444:
-----------------------------------------
steveloughran commented on code in PR #4869:
URL: https://github.com/apache/hadoop/pull/4869#discussion_r967342541
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java:
##########
@@ -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);
Review Comment:
use FileSystemTestHelper helper methods
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java:
##########
@@ -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));
Review Comment:
prefer ContractTestUtils assertions as they do listings on failures
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java:
##########
@@ -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 &&
Review Comment:
This is a messy way of passing a parameter down to the FS.
better to have a new getter/setter interface which viewfs can implement, and
a custom trash policy which does the work of using it.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java:
##########
@@ -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);
Review Comment:
use a different try/with/resources lifecycle to ensure a different fs is used
> Add Support for localized trash for ViewFileSystem in
> Trash.moveToAppropriateTrash
> ----------------------------------------------------------------------------------
>
> Key: HADOOP-18444
> URL: https://issues.apache.org/jira/browse/HADOOP-18444
> Project: Hadoop Common
> Issue Type: Bug
> Reporter: Xing Lin
> Assignee: Xing Lin
> Priority: Major
> Labels: pull-request-available
>
> Trash.moveToAppropriateTrash is used by _hadoop cli -rm_ and hive, to move
> files to trash. However, its current implementation does not support
> localized trash policy we added to ViewFileSystem in HADOOP-18144.
> The reason is in moveToAppropriateTrash, it first resolves a path and then
> uses the resolvedFs, to initialize the trash. As a result, it uses
> getTrashRoot() implementation from targetFs, not ViewFileSystem. The new
> localized trash policy we implemented in ViewFileSystem is not invoked.
> With the new localized trash policy for ViewFileSystem, the trash root would
> be local to a mount point, thus, for ViewFileSystem with this flag turned on,
> there is no need to resolve the path in moveToAppropriateTrash. Rename in
> ViewFileSystem can resolve the logical paths correctly and be able to move a
> file to trash within a mount point.
> Code section of current moveToAppropriateTrash implementation.
> {code:java}
> public static boolean moveToAppropriateTrash(FileSystem fs, Path p,
> Configuration conf) throws IOException {
> Path fullyResolvedPath = fs.resolvePath(p);
> FileSystem fullyResolvedFs =
> FileSystem.get(fullyResolvedPath.toUri(), conf);
> ...
> Trash trash = new Trash(fullyResolvedFs, conf);
> return trash.moveToTrash(fullyResolvedPath);
> }{code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
