HDFS-16791. Add getEnclosingRoot() API to filesystem interface and implementations #6198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
simbadzina
changed the title
4 tasks
goiri
reviewed
Oct 19, 2023
| @Test | ||
| public void testEnclosingRootEquivalence() throws IOException { | ||
| FileSystem fs = getFileSystem(); | ||
| Path root = path("/"); |
There was a problem hiding this comment.
Following up with the comments here.
My issue is that this unit tests should be the main unit test for this feature.
However, the test is very basic as the root is always /.
What can we do to make the benefit of the feature more obvious?
Do we always need to have fancier filesystems for this to be useful?
There was a problem hiding this comment.
I definitely could be wrong, but my current understanding is these are generic tests, in which case, I don't think there are more interesting tests. The functionality is for filesystems with mount points (ViewFileSystem and RBF) and encryption zones.
There are fancier tests in TestViewFileSystem and other specific implementations https://github.com/apache/hadoop/pull/6198/files#diff-c4eb93a08439a32bd178c44384d6bb15351780fd7bceba29d0aed3333f26a4cd
goiri
reviewed
Oct 19, 2023
...ain/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolServerSideTranslatorPB.java
Outdated
Show resolved
Hide resolved
| @@ -219,6 +221,9 @@ public void testTrashRootsAfterEncryptionZoneDeletion() throws Exception { | |||
| DFSTestUtil.verifyDelete(shell, fsTarget, zone, true); | |||
| assertTrue("ViewFileSystem trash roots should include EZ zone trash", | |||
| (fsView.getTrashRoots(true).size() == 2)); | |||
| } finally { | |||
| DFSTestUtil.deleteKey("test_key", cluster); | |||
There was a problem hiding this comment.
Can we do this in a separate JIRA?
There was a problem hiding this comment.
If i recall correctly I needed to add this because a test was failing. My new tests also use test keys, and i don't think this cleanup is correct between tests.
I can try and remove it thought and double check if you prefer (It just takes a long time for tests to run), but happy to do it
steveloughran
requested changes
Oct 19, 2023
...oop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java
Outdated
Show resolved
Hide resolved
...oop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
Outdated
Show resolved
Hide resolved
| try { | ||
| res = fsState.resolve((path.toString()), true); | ||
| } catch (FileNotFoundException ex) { | ||
| throw new NotInMountpointException(path, String.format("getEnclosingRoot - %s", ex.getMessage())); |
There was a problem hiding this comment.
add the caught exception as the cause
There was a problem hiding this comment.
im not totally sure what the ask is here, this is how i've been doing all of them. Are you saying only have the caught exception as the cause without the getEnclosingRoot - " part?
There was a problem hiding this comment.
no, I mean that .initCause() is needed to preserve the entire stack trace. otherwise root causes of problems may get lost.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md
Show resolved
Hide resolved
| #### Postconditions | ||
|
|
||
| * The path returned will not be null, if there is no deeper enclosing root, the root path ("/") will be returned. | ||
| * The path returned is a directory |
There was a problem hiding this comment.
can you add in the python-ish spec
path in ancestors(FS, p) or path == p
isDir(FS, p)There was a problem hiding this comment.
added, but tbh, don't totally understand the intent here, so i didn't correctly label it, i just copied it in as is.
Im guessing we want to add some context but not totally sure
There was a problem hiding this comment.
the python is trying to define the rules, the english is a wrapper around it. So think about how you'd convert those bullet points in terms of assertions you'd have before and after an implementation
added 2 commits
October 21, 2023 15:55
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran @goiri I fixed everything obvious and responded to comments I wasn't clear on. |
|
💔 -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. |
steveloughran
requested changes
Nov 3, 2023
...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
note, there is a new test failure but it is unrelated and being addressed in #6249 |
steveloughran
changed the title
|
merged. @mccormickt12 if you can do a PR for branch-3.3 with just this cherrypick, we can merge in there too once yetus approves. marking jira as fixed for 3.4 |
mccormickt12
added a commit
to mccormickt12/hadoop
that referenced
this pull request
Nov 9, 2023
…plementations (apache#6198) The enclosing root path is a common ancestor that should be used for temp and staging dirs as well as within encryption zones and other restricted directories. Contributed by Tom McCormick
This was referenced Nov 9, 2023
steveloughran
pushed a commit
that referenced
this pull request
Dec 1, 2023
…plementations (#6198) The enclosing root path is a common ancestor that should be used for temp and staging dirs as well as within encryption zones and other restricted directories. Contributed by Tom McCormick
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
…plementations (apache#6198) The enclosing root path is a common ancestor that should be used for temp and staging dirs as well as within encryption zones and other restricted directories. Contributed by Tom McCormick
NyteKnight
pushed a commit
to NyteKnight/hadoop
that referenced
this pull request
Jun 25, 2024
apache#367) * HDFS-16791. Add getEnclosingRoot() API to filesystem interface and implementations (apache#6198) The enclosing root path is a common ancestor that should be used for temp and staging dirs as well as within encryption zones and other restricted directories. Contributed by Tom McCormick ACLOVERRIDE due to OSS backport email issue * fixing merge conflicts * Fixing backport issues * ACLOVERRIDE due to OSS backport email issue --------- Co-authored-by: Tom <[email protected]> Co-authored-by: Tom McCormick <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HDFS-16791 Add getEnclosingRoot API to filesystem interface and all implementations
Description of PR
Add getEnclosingRoot API to filesystem interface.
This API is needed in ViewFileSystem and RBF setups to get around the rename constraint.
Renames (like custom trash and staging directories) should be done using this directory to ensure that each mount point is a self contained entity
How was this patch tested?
unit tests
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?