-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDFS-15492. Make trash root inside each snapshottable directory #2176
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good overall. The point of concern here is with each delete call with trash interval set on the filesystem, will potentially have one more rpc call to fetch the snapshottable dir list from namenode every time.
...ect/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/HdfsClientConfigKeys.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
public void testGetTrashRootsOnSnapshottableDirWithEncryptionZone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say we have a structure like /dir1/dir2 where dir1 is ez enabled and dir2 is made snapshottable. In such cases, anything deleted under dir2 will be under trash location under dir2 while everything which is deleted within dir1 but not dir2, will exist in trash under dir1. Will it lead to any issues??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a288110174c55b2c601b79b2beca9a27b2f34102, when a path given to getTrashRoot() is both inside an EZ and in a snapshottable dir, it should now choose the inner most trash.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
...hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
Outdated
Show resolved
Hide resolved
...hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
Show resolved
Hide resolved
@@ -516,6 +516,11 @@ | |||
public static final int | |||
DFS_NAMENODE_SNAPSHOT_SKIPLIST_MAX_SKIP_LEVELS_DEFAULT = 0; | |||
|
|||
public static final String DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED = | |||
"dfs.namenode.snapshot.trashroot.enabled"; | |||
public static final boolean DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED_DEFAULT = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess we need to define these configs in SnapshotManager if we intend not add it in hdfs-default.xml(which i would prefer). It leads to test failure here "hadoop.tools.TestHdfsConfigFields"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I will put it in FSNameSystem
as private config then as it is used there, similar to HDFS-15481 did.
Change-Id: I43aeb4959c5d0b9140eaed8f899b2f93f642eb04
Change-Id: I29120bf9c946841615c0e7efaac5c0056f1f795c
Change-Id: I7b497d7f10fcc9c37916abdab330c21c8bb05a16
…e on server-side only. Change-Id: I3d88fc5a437f8211755abe5d8b840de040a46099
Change-Id: Ic7c6078669925a3e13c6e462e6d0e51c3b9ff0fa
…ing snapshot on the test dir. Change-Id: I3dbe20785351b84375c644de6d06610c74d6e7d9
Change-Id: I6c65115eb8f176bc948f38f7c91e417391f8cab8
… null. Change-Id: Iff9a24d76966857340eacb8586c88d7b991468f9
…and EZ enabled to be added to the result twice; added test case for this. Change-Id: Iecad971f0712ad6d1183bdc14fc7a05864d9e84b
Change-Id: Ib023bd014afa1496b90c678645cb868d9056c9ed
…pshottable dir, it should choose the inner most trash. Change-Id: Ibf58de1031c94b0166f27f623eb5e9ea1669bac0
…eys, since it the NN will be the only source of truth for this config. Change-Id: I5247537803c2017b5af860e7eac057123031a86f
Change-Id: I6fd630e4d47fc9d403811d2bb92077322e7d4895
Change-Id: Id3c130c4f94508d21c656602a867fc59f2e6126a
…meSystem. Change-Id: Ife297d420156b9aafcb5d774ae419b6f3b6ce149
I rebased the commits on to the latest trunk. Also made |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Reran the test. Much less unrelated flaky test failures now. I'm merging this PR in a minute. |
I'm late to this. But we should verify to make sure httpfs also behaves the sames. Otherwise it'll break Hue. |
hey @jojochuang, thanks for checking in! I have written a simple test for verifying HttpFS (over WebHDFS). HttpFS works as expected. But I haven't figured out how to add a config to the HDFS cluster that |
…he#2176) Conflicts: hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelperClient.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java Change-Id: Ibe23d5760d61326865dad93460c6b426d002947d (cherry picked from commit 10ce000)
https://issues.apache.org/jira/browse/HDFS-15492
TODOs
getTrashRoot
getTrashRoots
getSnapshottableDirListing()
every time because the snapshot list can be large if there are a lot of snapshots on a cluster