[GitHub] [hadoop] ibuenros commented on a change in pull request #4034: HADOOP-18144. getTrashRoot/s in ViewFileSystem return a viewFS path

[GitBox <gi...@apache.org>]

ibuenros commented on a change in pull request #4034:
URL: https://github.com/apache/hadoop/pull/4034#discussion_r819904428



##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/TrashPolicyDefault.java
##########
@@ -191,8 +191,8 @@ public boolean moveToTrash(Path path) throws IOException {
         cause = e;
       }
     }
-    throw (IOException)
-      new IOException("Failed to move to trash: " + path).initCause(cause);
+    throw (IOException) new IOException(

Review comment:
       nit: Is this change intentional? i.e. removing the line break?

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/Constants.java
##########
@@ -134,8 +134,10 @@
       HCFSMountTableConfigLoader.class;
 
   /**
-   * Enable ViewFileSystem to return a trashRoot which is local to mount point.
+   * Enable ViewFileSystem to return a trashRoot which is in the root dir of a
+   * mount point.
    */
-  String CONFIG_VIEWFS_MOUNT_POINT_LOCAL_TRASH = "fs.viewfs.mount.point.local.trash";
-  boolean CONFIG_VIEWFS_MOUNT_POINT_LOCAL_TRASH_DEFAULT = false;
+  String CONFIG_VIEWFS_TRASH_ROOT_UNDER_MOUNT_POINT_ROOT =
+      "fs.viewfs.trash.root.under.mount.point.root";

Review comment:
       Why are we changing the configuration? Also @omalley what's Hadoop's policy on using dots as spaces as opposed to dots as hierarchical separators? i.e. should this be fs.viewfs.trash.rootUnderMountPointRoot?

##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java
##########
@@ -1141,60 +1143,52 @@ public void testTrashRootLocalizedTrash() throws IOException {
       fsView2.getTrashRoot(nonExistentPath);
     } catch (NotInMountpointException ignored) {
     }
-
-    // Case 5: path p is in the same mount point as targetFS.getTrashRoot().
-    // Return targetFS.getTrashRoot()
-    // Use a new Configuration object, so that we can start with an empty
-    // mount table. This would avoid a conflict between the /user link in
-    // setupMountPoints() and homeDir we will need to setup for this test.
-    // default homeDir for hdfs is /user/.
-    Configuration conf3 = ViewFileSystemTestSetup.createConfig();
-    Path homeDir = fsTarget.getHomeDirectory();
-    String homeParentDir = homeDir.getParent().toUri().getPath();
-    conf3.setBoolean(CONFIG_VIEWFS_MOUNT_POINT_LOCAL_TRASH, true);
-    ConfigUtil.addLink(conf3, homeParentDir,
-        new Path(targetTestRoot, homeParentDir).toUri());
-    Path homeTestPath = new Path(homeDir.toUri().getPath(), "testuser/file");
-    FileSystem fsView3 = FileSystem.get(FsConstants.VIEWFS_URI, conf3);
-    Assert.assertEquals(userTrashRoot, fsView3.getTrashRoot(homeTestPath));
   }
 
   /**
-   * A mocked FileSystem which returns a deep trash dir.
+   * A mocked FileSystem which returns overwrites getFileStatus() to always
+   * return an encrypted FileStatus.
    */
   static class MockTrashRootFS extends MockFileSystem {

Review comment:
       nit: make the name reflect the behavior, e.g. `AllPathsAreEncryptionRootsFS`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org