mkuchenbecker commented on code in PR #4967:
URL: https://github.com/apache/hadoop/pull/4967#discussion_r1086894151
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java:
##
@@ -1477,5 +1478,18 @@ public void setStoragePolicy(Path path, String
policyName)
throws IOException {
throw readOnlyMountTable("setStoragePolicy", path);
}
+
+@Override
+public Path getEnclosingRoot(Path path) throws IOException {
+ InodeTree.ResolveResult res;
+ try {
+res = fsState.resolve((path.toString()), true);
+ } catch (FileNotFoundException ex) {
+throw new NotInMountpointException(path, "getEnclosingRoot");
+ }
+ Path fullPath = new Path(res.resolvedPath);
+ Path enclosingPath = res.targetFileSystem.getEnclosingRoot(path);
+ return enclosingPath.depth() > fullPath.depth() ? enclosingPath :
fullPath;
Review Comment:
In what cases is full path returned and how would it be different from
enclosing?
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java:
##
@@ -4917,7 +4917,6 @@ public CompletableFuture build()
throws IOException {
*/
@InterfaceAudience.Public
@InterfaceStability.Unstable
- // Should this throw RuntimeException (instead of IO), so we can throw
NotInMountpointException from viewfs/rbf?
Review Comment:
Why remove this context?
##
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java:
##
@@ -0,0 +1,100 @@
+/**
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.contract;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class AbstractContractGetEnclosingRoot extends
AbstractFSContractTestBase {
+ private static final Logger LOG =
LoggerFactory.getLogger(AbstractContractGetEnclosingRoot.class);
+
+ @Override
+ public void setup() throws Exception {
+super.setup();
+ }
+
+ @Test
+ public void testEnclosingRootEquivalence() throws IOException {
+FileSystem fs = getFileSystem();
+Path root = path("/");
+Path foobar = path("/foo/bar");
+
+assertEquals(fs.getEnclosingRoot(foobar), root);
+assertEquals(fs.getEnclosingRoot(fs.getEnclosingRoot(foobar)), root);
+assertEquals(fs.getEnclosingRoot(foobar), fs.getEnclosingRoot(root));
+
+assertEquals(fs.getEnclosingRoot(path(foobar.toString())), root);
+
assertEquals(fs.getEnclosingRoot(fs.getEnclosingRoot(path(foobar.toString(,
root);
+assertEquals(fs.getEnclosingRoot(path(foobar.toString())),
fs.getEnclosingRoot(root));
+ }
+
+ @Test
+ public void testEnclosingRootPathExists() throws Exception {
Review Comment:
I'm not sure I understand the value of this test vs the other tests. Can you
add some details about what this is testing. i.e. in almost all cases we
basically want "/foo/bar" or whatever path to almost always return "/" as the
path. I'm not seeing how this is different.
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java:
##
@@ -1477,5 +1478,18 @@ public void setStoragePolicy(Path path, String
policyName)
throws IOException {
throw readOnlyMountTable("setStoragePolicy", path);
}
+
+@Override
+public Path getEnclosingRoot(Path path) throws IOException {
+ InodeTree.ResolveResult res;
+ try {
+res = fsState.resolve((path.toString()), true);
+ } catch (FileNotFoundException ex) {
+throw new NotInMountpointException(path, "getEnclosingRoot");
Review Comment:
You should propagate the original error as the cause of the thrown error.
##
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEnclosingRoot.java:
##
@@ -92,11 +92,14 @@ protected void setProvider() {
@After
public void teardown() {
-if
