HADOOP-18193:Support nested mount points in INodeTree #4181
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
mccormickt12
suggested changes
Apr 15, 2022
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestNestedMountPoint.java
Show resolved
Hide resolved
|
|
||
| @Test | ||
| public void testMultiNestedMountPointsPathResolveToDirLink() throws Exception { | ||
| // /a/b/f resolves to /a/b and /f |
There was a problem hiding this comment.
Here you have a test that tests a top level dirlink correctly picks the correct link instead of a nested mount point.
Since essentially unlimited nesting of these dirlinks is supported, can you add another test that tests that a nested mount point also handles this scenario correctly
| @@ -167,8 +169,21 @@ void setupMountPoints() { | |||
| new Path(targetTestRoot, "missingTarget").toUri()); | |||
| ConfigUtil.addLink(conf, "/linkToAFile", | |||
| new Path(targetTestRoot, "aFile").toUri()); | |||
|
|
|||
| // Enable nested mount point, ViewFilesystem should support both non-nested and nested mount points | |||
| ConfigUtil.setIsNestedMountPointSupported(conf, true); | |||
There was a problem hiding this comment.
this is going to affect all the other tests here right?
We might want to keep the testing of nested mount points separate
There was a problem hiding this comment.
+1 to this. it would be good to have tests specific to mount points.
There was a problem hiding this comment.
Can you create a new test case in this class for these? It would be good to keep the nested mount points logic separate
| @@ -81,6 +88,19 @@ enum ResultKind { | |||
| private List<RegexMountPoint<T>> regexMountPointList = | |||
| new ArrayList<RegexMountPoint<T>>(); | |||
|
|
|||
| private final boolean isNestedMountPointSupported; | |||
| private Set<LinkEntry> sortedLinkEntries = new TreeSet<>((o1, o2) -> { | |||
There was a problem hiding this comment.
its a little weird that sortedLinkEntries is a private member, but only used in getLinkEntries which just does an addAll and return. I think we should probably get rid of this as a member.
otherwise we need to consider clearing the set. It also just seems like an unnecessary memory overhead.
There was a problem hiding this comment.
I agree we can get rid of a a member. I originally used it as a member but later return in a function and didn't remove it.
virajith
reviewed
Apr 15, 2022
There was a problem hiding this comment.
Didn't review the code but a large part of the diff here is changes to spacing - can you avoid this? Try the instructions here: https://cwiki.apache.org/confluence/display/hadoop/how+to+contribute#HowToContribute-IntegratedDevelopmentEnvironment(IDE)
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
li-leyang
force-pushed
the
HADOOP-18193
branch
7 times, most recently
from
52504a5
to
3723e27
Compare
|
💔 -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. |
|
💔 -1 overall
This message was automatically generated. |
virajith
requested changes
Apr 21, 2022
| @@ -247,4 +247,22 @@ public static String getDefaultMountTableName(final Configuration conf) { | |||
| return conf.get(Constants.CONFIG_VIEWFS_DEFAULT_MOUNT_TABLE_NAME_KEY, | |||
| Constants.CONFIG_VIEWFS_DEFAULT_MOUNT_TABLE); | |||
| } | |||
|
|
|||
| /** | |||
| * Get the bool config whether nested mount point is supported. | |||
There was a problem hiding this comment.
Nit - "Get the bool config" -> "Check" - you aren't really getting a config
There was a problem hiding this comment.
this is to get config value from conf object. I will change it to "check".
| @@ -133,6 +138,9 @@ public INode(String pathToNode, UserGroupInformation aUgi) { | |||
| // and is read only. | |||
| abstract boolean isInternalDir(); | |||
|
|
|||
| // Identity some type of inodes(nested mount point). | |||
There was a problem hiding this comment.
Nit: Use multi-line comment. The other methods here seem to be off.
There was a problem hiding this comment.
Also, "Identity some type of inodes" is very vague - can you describe this more crisply?
There was a problem hiding this comment.
yeah, will expand this comment
| } | ||
|
|
||
| /** | ||
| * Internal class to represent nested mount points in INodeTree. Nested mount points are non-leaf nodes in INode tree. |
There was a problem hiding this comment.
Can you define "nested mount points" clearly here? The statement assumes a certain definition - be more concrete
There was a problem hiding this comment.
I have added in the comments.
| break; | ||
| default: | ||
| throw new IllegalArgumentException(linkType + ": Infeasible linkType"); | ||
| case SINGLE: |
There was a problem hiding this comment.
avoid this change - not required
There was a problem hiding this comment.
Removed this change
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Show resolved
Hide resolved
| @@ -167,8 +169,21 @@ void setupMountPoints() { | |||
| new Path(targetTestRoot, "missingTarget").toUri()); | |||
| ConfigUtil.addLink(conf, "/linkToAFile", | |||
| new Path(targetTestRoot, "aFile").toUri()); | |||
|
|
|||
| // Enable nested mount point, ViewFilesystem should support both non-nested and nested mount points | |||
| ConfigUtil.setIsNestedMountPointSupported(conf, true); | |||
There was a problem hiding this comment.
+1 to this. it would be good to have tests specific to mount points.
...on-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestNestedMountPoint.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Show resolved
Hide resolved
li-leyang
force-pushed
the
HADOOP-18193
branch
4 times, most recently
from
52de98d
to
329eb9a
Compare
|
💔 -1 overall
This message was automatically generated. |
virajith
requested changes
May 6, 2022
| } | ||
|
|
||
| /** | ||
| * Internal class to represent a INodeDir which also contains a INodeLink. This is used to support nested mount point |
There was a problem hiding this comment.
nit:
"support nested mount point where a INode is internalDir but would also point to a mount link" ->
"support nested mount points where an INode is an internalDir and points to a mount link."
| @@ -682,6 +748,32 @@ protected InodeTree(final Configuration config, final String viewName, | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Get collection of linkEntry. If nested mount point is supported, sort mount point src path based on alphabetical order. | |||
There was a problem hiding this comment.
nit: "sort mount point src path based on alphabetical order." -> "sort mount points based on alphabetical order of the src paths"
| @@ -682,6 +748,32 @@ protected InodeTree(final Configuration config, final String viewName, | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Get collection of linkEntry. If nested mount point is supported, sort mount point src path based on alphabetical order. | |||
| * The purpose is to group nested path(shortest path always comes first) during INodeTree creation. | |||
There was a problem hiding this comment.
nit: "nested path(shortest" -> "nested paths (shortest"
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Show resolved
Hide resolved
| @Test | ||
| public void testCreateNonRecursive() throws IOException { | ||
| Path path = fileSystemTestHelper.getTestRootPath(fsView, "/user/foo"); | ||
| fsView.createNonRecursive(path, false, 1024, (short)1, 1024L, null); | ||
| FileStatus status = fsView.getFileStatus(new Path("/user/foo")); | ||
| Assert.assertTrue("Created file should be type file", | ||
| fsView.isFile(new Path("/user/foo"))); | ||
| fsView.getFileStatus(new Path("/user/foo")).isFile()); |
| Assert.assertTrue("Target of created file should be type file", | ||
| fsTarget.isFile(new Path(targetTestRoot, "user/foo"))); | ||
| fsTarget.getFileStatus(new Path(targetTestRoot, "user/foo")).isFile()); |
| // Create file with a 3 component dirs | ||
| fileSystemTestHelper.createFile(fsView, "/internalDir/internalDir2/linkToDir3/foo"); | ||
| Assert.assertTrue("Created file should be type file", | ||
| fsView.isFile(new Path("/internalDir/internalDir2/linkToDir3/foo"))); | ||
| fsView.getFileStatus(new Path("/internalDir/internalDir2/linkToDir3/foo")).isFile()); |
There was a problem hiding this comment.
you seem to have replaced a bunch of deprecated calls here - don't mix that up with this PR. You can have a separate PR to clean this up. It confuses the reader as to how the changes are related to the current change.
There was a problem hiding this comment.
removed deprecated call changes
| @@ -167,8 +169,21 @@ void setupMountPoints() { | |||
| new Path(targetTestRoot, "missingTarget").toUri()); | |||
| ConfigUtil.addLink(conf, "/linkToAFile", | |||
| new Path(targetTestRoot, "aFile").toUri()); | |||
|
|
|||
| // Enable nested mount point, ViewFilesystem should support both non-nested and nested mount points | |||
| ConfigUtil.setIsNestedMountPointSupported(conf, true); | |||
There was a problem hiding this comment.
Can you create a new test case in this class for these? It would be good to keep the nested mount points logic separate
| ConfigUtil.addLink(conf, "/data/dataB", | ||
| new Path(targetTestRoot, "user").toUri()); | ||
| ConfigUtil.addLink(conf, "/internalDir/linkToDir2/linkToDir2", | ||
| new Path(targetTestRoot,"linkToDir2").toUri()); |
There was a problem hiding this comment.
Add a test for listStatus?
There was a problem hiding this comment.
I created a new test class that extends ViewFileSystemBaseTest
There was a problem hiding this comment.
I have moved nested mount point tests to ViewFileSystemBaseTest
|
💔 -1 overall
This message was automatically generated. |
omalley
reviewed
May 10, 2022
| * @return whether nested mount point is supported | ||
| */ | ||
| public static boolean isNestedMountPointSupported(final Configuration conf) { | ||
| return conf.getBoolean(Constants.CONFIG_NESTED_MOUNT_POINT_SUPPORTED, false); |
There was a problem hiding this comment.
I think that the default should be true here, because this feature should be enabled by default.
omalley
reviewed
May 10, 2022
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Show resolved
Hide resolved
omalley
reviewed
May 10, 2022
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Outdated
Show resolved
Hide resolved
omalley
reviewed
May 11, 2022
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
…e, always sort linkEntries in INodeTree
omalley
requested changes
May 11, 2022
There was a problem hiding this comment.
I'd suggest adding getLink to Inode. See https://github.com/omalley/hadoop/tree/hadoop-18193 or more precisely omalley@6733b57
Changed per your suggestion |
omalley
requested changes
May 11, 2022
@omalley I have added multi-level nested mount point like below in the TestNestedMountPoint#setup section so it should be covered, unless you mean something else. /a/b |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@omalley - looks like this was merged but PR is marked as closed. I believe the convention is to "squash merge" the PR and the PR will be marked as "Merged". Otherwise, it's confusing if the PR has been rejected or not. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
Fixes apache#4181 Signed-off-by: Owen O'Malley <oomalley@linkedin.com>
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.
Description of PR
https://issues.apache.org/jira/browse/HADOOP-18193
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?