omalley commented on a change in pull request #4034:
URL: https://github.com/apache/hadoop/pull/4034#discussion_r819927130
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
##########
@@ -1128,51 +1128,58 @@ public BlockStoragePolicySpi getStoragePolicy(Path src)
throws IOException {
return allPolicies;
}
+ // Test whether a path is inside a snapshot or encrypted.
+ boolean isSnapshotEnabledOrEncrypted(Path p) throws IOException {
+ try {
+ FileStatus status = getFileStatus(p);
+ return status.isSnapshotEnabled() || status.isEncrypted();
Review comment:
There is more to the current code than this, and it is important. In
particular, the snapshot only matters if they want the snapshot trash to stay
inside the snapshot.
##########
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:
Agreed. More consistent would be: fs.viewfs.trash.under-mount-root
##########
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:
+1
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
##########
@@ -1128,51 +1128,58 @@ public BlockStoragePolicySpi getStoragePolicy(Path src)
throws IOException {
return allPolicies;
}
+ // Test whether a path is inside a snapshot or encrypted.
+ boolean isSnapshotEnabledOrEncrypted(Path p) throws IOException {
+ try {
+ FileStatus status = getFileStatus(p);
+ return status.isSnapshotEnabled() || status.isEncrypted();
Review comment:
Just in general, it isn't good to try to replicate the underlying
filesystem's features.
##########
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:
Please revert this change.
--
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-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
