You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/04/21 00:57:26 UTC
[GitHub] [hadoop] virajith commented on a diff in pull request #4181: HADOOP-18193:Support nested mount points in INodeTree
virajith commented on code in PR #4181:
URL: https://github.com/apache/hadoop/pull/4181#discussion_r854653058
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ConfigUtil.java:
##########
@@ -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.
Review Comment:
Nit - "Get the bool config" -> "Check" - you aren't really getting a config
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java:
##########
@@ -212,6 +225,33 @@ void addLink(final String pathComponent, final INodeLink<T> link)
}
children.put(pathComponent, link);
}
+
+ void addDirLink(final String pathComponent, final INodeDirLink<T> dirLink) {
+ children.put(pathComponent, dirLink);
+ }
+ }
+
+ /**
+ * Internal class to represent nested mount points in INodeTree. Nested mount points are non-leaf nodes in INode tree.
Review Comment:
Can you define "nested mount points" clearly here? The statement assumes a certain definition - be more concrete
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java:
##########
@@ -403,24 +455,24 @@ private void createLink(final String src, final String target,
final String fullPath = curInode.fullPath + (curInode == root ? "" : "/")
+ iPath;
switch (linkType) {
- case SINGLE:
- newLink = new INodeLink<T>(fullPath, aUgi,
- initAndGetTargetFs(), target);
- break;
- case SINGLE_FALLBACK:
- case MERGE_SLASH:
- // Link fallback and link merge slash configuration
- // are handled specially at InodeTree.
- throw new IllegalArgumentException("Unexpected linkType: " + linkType);
- case MERGE:
- case NFLY:
- final String[] targetUris = StringUtils.getStrings(target);
- newLink = new INodeLink<T>(fullPath, aUgi,
- getTargetFileSystem(settings, StringUtils.stringToURI(targetUris)),
- targetUris);
- break;
- default:
- throw new IllegalArgumentException(linkType + ": Infeasible linkType");
+ case SINGLE:
Review Comment:
avoid this change - not required
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java:
##########
@@ -682,6 +736,25 @@ protected InodeTree(final Configuration config, final String viewName,
}
}
+ private Collection<LinkEntry> getLinkEntries(List<LinkEntry> linkEntries) {
+ if (isNestedMountPointSupported) {
Review Comment:
add a comment on why you are sorting them with this change?
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java:
##########
@@ -682,6 +736,25 @@ protected InodeTree(final Configuration config, final String viewName,
}
}
+ private Collection<LinkEntry> getLinkEntries(List<LinkEntry> linkEntries) {
+ if (isNestedMountPointSupported) {
+ Set<LinkEntry> sortedLinkEntries = new TreeSet<>((o1, o2) -> {
+ if (o1 == null) {
+ return -1;
+ }
+ if (o2 == null) {
+ return 1;
+ }
+ String src1 = o1.getSrc().toLowerCase();
Review Comment:
Also, is lower casing the right way to do this? shouldn't you just compare the input Strings as /Foo/Bar and /foo/bar are different paths?
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java:
##########
@@ -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).
Review Comment:
Nit: Use multi-line comment. The other methods here seem to be off.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/ViewFileSystemBaseTest.java:
##########
@@ -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);
Review Comment:
+1 to this. it would be good to have tests specific to mount points.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java:
##########
@@ -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).
Review Comment:
Also, "Identity some type of inodes" is very vague - can you describe this more crisply?
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java:
##########
@@ -212,6 +225,33 @@ void addLink(final String pathComponent, final INodeLink<T> link)
}
children.put(pathComponent, link);
}
+
+ void addDirLink(final String pathComponent, final INodeDirLink<T> dirLink) {
+ children.put(pathComponent, dirLink);
+ }
+ }
+
+ /**
+ * Internal class to represent nested mount points in INodeTree. Nested mount points are non-leaf nodes in INode tree.
+ * Hence it inherits INodeDir. But it has additional attributes so wrap it with link.
+ * @param <T>
+ */
+ static class INodeDirLink<T> extends INodeDir<T> {
Review Comment:
Can you have a comment on what are the semantics of nested mount points? Currently, it's implicit in the code and the unit tests - it would be good to have an explicit declaration of this.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java:
##########
@@ -212,6 +225,33 @@ void addLink(final String pathComponent, final INodeLink<T> link)
}
children.put(pathComponent, link);
}
+
+ void addDirLink(final String pathComponent, final INodeDirLink<T> dirLink) {
+ children.put(pathComponent, dirLink);
+ }
+ }
+
+ /**
+ * Internal class to represent nested mount points in INodeTree. Nested mount points are non-leaf nodes in INode tree.
+ * Hence it inherits INodeDir. But it has additional attributes so wrap it with link.
+ * @param <T>
+ */
+ static class INodeDirLink<T> extends INodeDir<T> {
Review Comment:
What should be the semantics of {{isLink()}} for this? Seems inconsistent in the current implementation as this is a link and an internal directory but isLink() would return false on this?
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java:
##########
@@ -377,9 +422,16 @@ private void createLink(final String src, final String target,
nextInode = newDir;
}
if (nextInode.isLink()) {
- // Error - expected a dir but got a link
- throw new FileAlreadyExistsException("Path " + nextInode.fullPath +
- " already exists as link");
+ if (isNestedMountPointSupported) {
+ // nested mount detected, add a new INodeDirLink that wraps existing INodeLink to INodeTree and override existing INodelink
+ INodeDirLink<T> dirLink = new INodeDirLink<T>(nextInode.fullPath, aUgi, (INodeLink<T>) nextInode);
Review Comment:
For creating the dir link, follow the same pattern as what exists in the above if clause of if (nextInode == null) (i.e. addDirLink creates a new dir link and returns the new INodeDirLink). Is there a reason to deviate? also, what shouldn't setInternalDirFs be called here?
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestNestedMountPoint.java:
##########
@@ -0,0 +1,362 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.fs.viewfs;
+
+import java.net.URI;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FsConstants;
+import org.apache.hadoop.fs.Path;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+
+public class TestNestedMountPoint {
+ private InodeTree inodeTree;
+ private Configuration conf;
+ private String mtName;
+ private URI fsUri;
+
+ static class TestNestMountPointFileSystem {
+ public URI getUri() {
+ return uri;
+ }
+
+ private URI uri;
+
+ TestNestMountPointFileSystem(URI uri) {
+ this.uri = uri;
+ }
+ }
+
+ static class TestNestMountPointInternalFileSystem extends TestNestMountPointFileSystem {
+ TestNestMountPointInternalFileSystem(URI uri) {
+ super(uri);
+ }
+ }
+
+ private static final URI LINKFALLBACK_TARGET = URI.create("hdfs://nn00");
+ private static final URI NN1_TARGET = URI.create("hdfs://nn01/a/b");
+ private static final URI NN2_TARGET = URI.create("hdfs://nn02/a/b/e");
+ private static final URI NN3_TARGET = URI.create("hdfs://nn03/a/b/c/d");
+ private static final URI NN4_TARGET = URI.create("hdfs://nn04/a/b/c/d/e");
+ private static final URI NN5_TARGET = URI.create("hdfs://nn05/b/c/d/e");
+ private static final URI NN6_TARGET = URI.create("hdfs://nn06/b/c/d/e/f");
+
+ @Before
+ public void setUp() throws Exception {
+ conf = new Configuration();
+ mtName = TestNestedMountPoint.class.getName();
+ ConfigUtil.setIsNestedMountPointSupported(conf, true);
+ ConfigUtil.addLink(conf, mtName, "/a/b", NN1_TARGET);
+ ConfigUtil.addLink(conf, mtName, "/a/b/e", NN2_TARGET);
+ ConfigUtil.addLink(conf, mtName, "/a/b/c/d", NN3_TARGET);
+ ConfigUtil.addLink(conf, mtName, "/a/b/c/d/e", NN4_TARGET);
+ ConfigUtil.addLink(conf, mtName, "/b/c/d/e", NN5_TARGET);
+ ConfigUtil.addLink(conf, mtName, "/b/c/d/e/f", NN6_TARGET);
+ ConfigUtil.addLinkFallback(conf, mtName, LINKFALLBACK_TARGET);
+
+ fsUri = new URI(FsConstants.VIEWFS_SCHEME, mtName, "/", null, null);
+
+ inodeTree = new InodeTree<TestNestedMountPoint.TestNestMountPointFileSystem>(conf,
+ mtName, fsUri, false) {
+ @Override
+ protected Function<URI, TestNestedMountPoint.TestNestMountPointFileSystem> initAndGetTargetFs() {
+ return new Function<URI, TestNestMountPointFileSystem>() {
+ @Override
+ public TestNestedMountPoint.TestNestMountPointFileSystem apply(URI uri) {
+ return new TestNestMountPointFileSystem(uri);
+ }
+ };
+ }
+
+ // For intenral dir fs
+ @Override
+ protected TestNestedMountPoint.TestNestMountPointInternalFileSystem getTargetFileSystem(
+ final INodeDir<TestNestedMountPoint.TestNestMountPointFileSystem> dir) {
+ return new TestNestMountPointInternalFileSystem(fsUri);
+ }
+
+ @Override
+ protected TestNestedMountPoint.TestNestMountPointInternalFileSystem getTargetFileSystem(
+ final String settings, final URI[] mergeFsURIList) {
+ return new TestNestMountPointInternalFileSystem(null);
+ }
+ };
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ inodeTree = null;
+ }
+
+ @Test
+ public void testPathResolveToLink() throws Exception {
+ // /a/b/c/d/e/f resolves to /a/b/c/d/e and /f
+ InodeTree.ResolveResult resolveResult = inodeTree.resolve("/a/b/c/d/e/f", true);
+ Assert.assertEquals(InodeTree.ResultKind.EXTERNAL_DIR, resolveResult.kind);
+ Assert.assertEquals("/a/b/c/d/e", resolveResult.resolvedPath);
+ Assert.assertEquals(new Path("/f"), resolveResult.remainingPath);
+ Assert.assertTrue(resolveResult.targetFileSystem instanceof TestNestMountPointFileSystem);
+ Assert.assertEquals(NN4_TARGET, ((TestNestMountPointFileSystem) resolveResult.targetFileSystem).getUri());
+ Assert.assertTrue(resolveResult.isLastInternalDirLink());
+
+ // /a/b/c/d/e resolves to /a/b/c/d/e and /
+ InodeTree.ResolveResult resolveResult2 = inodeTree.resolve("/a/b/c/d/e", true);
+ Assert.assertEquals(InodeTree.ResultKind.EXTERNAL_DIR, resolveResult2.kind);
+ Assert.assertEquals("/a/b/c/d/e", resolveResult2.resolvedPath);
+ Assert.assertEquals(new Path("/"), resolveResult2.remainingPath);
+ Assert.assertTrue(resolveResult2.targetFileSystem instanceof TestNestMountPointFileSystem);
+ Assert.assertEquals(NN4_TARGET, ((TestNestMountPointFileSystem) resolveResult2.targetFileSystem).getUri());
+ Assert.assertTrue(resolveResult2.isLastInternalDirLink());
+
+ // /a/b/c/d/e/f/g/h/i resolves to /a/b/c/d/e and /f/g/h/i
+ InodeTree.ResolveResult resolveResult3 = inodeTree.resolve("/a/b/c/d/e/f/g/h/i", true);
+ Assert.assertEquals(InodeTree.ResultKind.EXTERNAL_DIR, resolveResult3.kind);
+ Assert.assertEquals("/a/b/c/d/e", resolveResult3.resolvedPath);
+ Assert.assertEquals(new Path("/f/g/h/i"), resolveResult3.remainingPath);
+ Assert.assertTrue(resolveResult3.targetFileSystem instanceof TestNestMountPointFileSystem);
+ Assert.assertEquals(NN4_TARGET, ((TestNestMountPointFileSystem) resolveResult3.targetFileSystem).getUri());
+ Assert.assertTrue(resolveResult3.isLastInternalDirLink());
+ }
+
+ @Test
+ public void testPathResolveToLink_NotResolveLastComponent() throws Exception {
+ // /a/b/c/d/e/f resolves to /a/b/c/d/e and /f
+ InodeTree.ResolveResult resolveResult = inodeTree.resolve("/a/b/c/d/e/f", false);
+ Assert.assertEquals(InodeTree.ResultKind.EXTERNAL_DIR, resolveResult.kind);
+ Assert.assertEquals("/a/b/c/d/e", resolveResult.resolvedPath);
+ Assert.assertEquals(new Path("/f"), resolveResult.remainingPath);
+ Assert.assertTrue(resolveResult.targetFileSystem instanceof TestNestMountPointFileSystem);
+ Assert.assertEquals(NN4_TARGET, ((TestNestMountPointFileSystem) resolveResult.targetFileSystem).getUri());
+ Assert.assertTrue(resolveResult.isLastInternalDirLink());
+
+ // /a/b/c/d/e resolves to /a/b/c/d and /e
+ InodeTree.ResolveResult resolveResult2 = inodeTree.resolve("/a/b/c/d/e", false);
+ Assert.assertEquals(InodeTree.ResultKind.EXTERNAL_DIR, resolveResult2.kind);
+ Assert.assertEquals("/a/b/c/d", resolveResult2.resolvedPath);
+ Assert.assertEquals(new Path("/e"), resolveResult2.remainingPath);
+ Assert.assertTrue(resolveResult2.targetFileSystem instanceof TestNestMountPointFileSystem);
+ Assert.assertEquals(NN3_TARGET, ((TestNestMountPointFileSystem) resolveResult2.targetFileSystem).getUri());
+ Assert.assertTrue(resolveResult2.isLastInternalDirLink());
+
+ // /a/b/c/d/e/f/g/h/i resolves to /a/b/c/d/e and /f/g/h/i
+ InodeTree.ResolveResult resolveResult3 = inodeTree.resolve("/a/b/c/d/e/f/g/h/i", false);
+ Assert.assertEquals(InodeTree.ResultKind.EXTERNAL_DIR, resolveResult3.kind);
+ Assert.assertEquals("/a/b/c/d/e", resolveResult3.resolvedPath);
+ Assert.assertEquals(new Path("/f/g/h/i"), resolveResult3.remainingPath);
+ Assert.assertTrue(resolveResult3.targetFileSystem instanceof TestNestMountPointFileSystem);
+ Assert.assertEquals(NN4_TARGET, ((TestNestMountPointFileSystem) resolveResult3.targetFileSystem).getUri());
+ Assert.assertTrue(resolveResult3.isLastInternalDirLink());
+ }
+
+ @Test
+ public void testPathResolveToDirLink() throws Exception {
+ // /a/b/c/d/f resolves to /a/b/c/d, /f
+ InodeTree.ResolveResult resolveResult = inodeTree.resolve("/a/b/c/d/f", true);
+ Assert.assertEquals(InodeTree.ResultKind.EXTERNAL_DIR, resolveResult.kind);
+ Assert.assertEquals("/a/b/c/d", resolveResult.resolvedPath);
+ Assert.assertEquals(new Path("/f"), resolveResult.remainingPath);
+ Assert.assertTrue(resolveResult.targetFileSystem instanceof TestNestMountPointFileSystem);
+ Assert.assertEquals(NN3_TARGET, ((TestNestMountPointFileSystem) resolveResult.targetFileSystem).getUri());
+ Assert.assertTrue(resolveResult.isLastInternalDirLink());
+
+ // /a/b/c/d resolves to /a/b/c/d and /
+ InodeTree.ResolveResult resolveResult2 = inodeTree.resolve("/a/b/c/d", true);
+ Assert.assertEquals(InodeTree.ResultKind.EXTERNAL_DIR, resolveResult2.kind);
+ Assert.assertEquals("/a/b/c/d", resolveResult2.resolvedPath);
+ Assert.assertEquals(new Path("/"), resolveResult2.remainingPath);
+ Assert.assertTrue(resolveResult2.targetFileSystem instanceof TestNestMountPointFileSystem);
+ Assert.assertEquals(NN3_TARGET, ((TestNestMountPointFileSystem) resolveResult2.targetFileSystem).getUri());
+ Assert.assertTrue(resolveResult2.isLastInternalDirLink());
+
+ // /a/b/c/d/f/g/h/i resolves to /a/b/c/d and /f/g/h/i
+ InodeTree.ResolveResult resolveResult3 = inodeTree.resolve("/a/b/c/d/f/g/h/i", true);
+ Assert.assertEquals(InodeTree.ResultKind.EXTERNAL_DIR, resolveResult3.kind);
+ Assert.assertEquals("/a/b/c/d", resolveResult3.resolvedPath);
+ Assert.assertEquals(new Path("/f/g/h/i"), resolveResult3.remainingPath);
+ Assert.assertTrue(resolveResult3.targetFileSystem instanceof TestNestMountPointFileSystem);
+ Assert.assertEquals(NN3_TARGET, ((TestNestMountPointFileSystem) resolveResult3.targetFileSystem).getUri());
+ Assert.assertTrue(resolveResult3.isLastInternalDirLink());
+ }
+
+ @Test
+ public void testPathResolveToDirLink_NotResolveLastComponent() throws Exception {
Review Comment:
nit - avoid the use of _
--
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-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org