-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HADOOP-18193:Support nested mount points in INodeTree #4181
Conversation
...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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this. it would be good to have tests specific to mount points.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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. |
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. |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - "Get the bool config" -> "Check" - you aren't really getting a config
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Use multi-line comment. The other methods here seem to be off.
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.
Also, "Identity some type of inodes" is very vague - can you describe this more crisply?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you define "nested mount points" clearly here? The statement assumes a certain definition - be more concrete
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 have added in the comments.
break; | ||
default: | ||
throw new IllegalArgumentException(linkType + ": Infeasible linkType"); | ||
case SINGLE: |
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.
avoid this change - not required
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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
52de98d
to
329eb9a
Compare
💔 -1 overall
This message was automatically generated. |
} | ||
|
||
/** | ||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
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.
Fixed
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "sort mount point src path based on alphabetical order." -> "sort mount points based on alphabetical order of the src paths"
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.
Fixed
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "nested path(shortest" -> "nested paths (shortest"
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.
Fixed
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()); |
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.
avoid - not related
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.
removed
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()); |
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.
avoid - not related
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.
removed
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test for listStatus?
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 created a new test class that extends ViewFileSystemBaseTest
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.
test
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 have moved nested mount point tests to ViewFileSystemBaseTest
💔 -1 overall
This message was automatically generated. |
* @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the default should be true here, because this feature should be enabled by 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.
this is fair
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
Outdated
Show resolved
Hide resolved
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
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'd suggest adding getLink to Inode. See https://github.com/omalley/hadoop/tree/hadoop-18193 or more precisely omalley@6733b57
Changed per your suggestion |
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.
This is looking good, thanks, Lei.
Can you add a test case that looks like:
/a
/a/b/c/d
/a/b/c/d/e/f/g
I'd like a test that makes sure that multiple levels work correctly.
@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. |
Fixes apache#4181 Signed-off-by: Owen O'Malley <oomalley@linkedin.com>
Description of PR
https://issues.apache.org/jira/browse/HADOOP-18193
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?