[
https://issues.apache.org/jira/browse/HADOOP-18193?focusedWorklogId=759682&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-759682
]
ASF GitHub Bot logged work on HADOOP-18193:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 21/Apr/22 00:57
Start Date: 21/Apr/22 00:57
Worklog Time Spent: 10m
Work Description: 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 _
Issue Time Tracking
-------------------
Worklog Id: (was: 759682)
Time Spent: 2h 50m (was: 2h 40m)
> Support nested mount points in INodeTree
> ----------------------------------------
>
> Key: HADOOP-18193
> URL: https://issues.apache.org/jira/browse/HADOOP-18193
> Project: Hadoop Common
> Issue Type: Improvement
> Components: viewfs
> Affects Versions: 2.10.0
> Reporter: Lei Yang
> Assignee: Lei Yang
> Priority: Major
> Labels: pull-request-available
> Time Spent: 2h 50m
> Remaining Estimate: 0h
>
> Defining following client mount table config is not supported in INodeTree
> and will throw FileAlreadyExistsException
> fs.viewfs.mounttable.link./foo/bar=hdfs://nn1/foo/bar
> fs.viewfs.mounttable.link./foo=hdfs://nn02/foo
>
> INodeTree has 2 methods that need change to support nested mount points.
> createLink(..): build INodeTree during fs init.
> resolve(..): resolve path in INodeTree with viewfs apis.
>
> ViewFileSystem and ViewFs referes INodeTree.resolve(..) to resolve path to
> specific mount point. No changes are expected in both classes. However, we
> need to support existing use cases and make sure no regression are caused.
>
> AC:
> # INodeTree.createlink should support creating nested mount
> points.(INodeTree is constructed during fs init)
> # INodeTree.resolve should support resolve path based on nested mount
> points. (INodeTree.resolve is used in viewfs apis)
> # No regression in existing ViewFileSystem and ViewFs apis.
> # Ensure some important apis are not broken with nested mount points.
> (Rename, getContentSummary, listStatus...)
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
