fixing pr comments

apache · Oct 16, 2023 · 7f7bdbd · 7f7bdbd
1 parent 447d217
commit 7f7bdbd
Show file tree

Hide file tree

Showing 7 changed files with 63 additions and 49 deletions.
diff --git a/...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java b/...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java
@@ -1652,8 +1652,8 @@ public MultipartUploaderBuilder createMultipartUploader(Path basePath)
   @InterfaceAudience.Public
   @InterfaceStability.Unstable
   public Path getEnclosingRoot(Path path) throws IOException {
-    this.makeQualified(path);
-    return this.makeQualified(new Path("/"));
+    makeQualified(path);
+    return makeQualified(new Path("/"));
   }
 
   /**

diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
@@ -4913,7 +4913,7 @@ public CompletableFuture<FSDataInputStream> build() throws IOException {
    *
    * @param path file path to find the enclosing root path for
    * @return a path to the enclosing root
-   * @throws IOException
+   * @throws IOException early checks like failure to resolve path cause IO failures
    */
   @InterfaceAudience.Public
   @InterfaceStability.Unstable

diff --git a/...ommon-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java b/...ommon-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
@@ -1376,7 +1376,7 @@ public Path getEnclosingRoot(Path path) throws IOException {
     try {
       res = fsState.resolve(getUriPath(path), true);
     } catch (FileNotFoundException ex) {
-      throw new NotInMountpointException(path, "getEnclosingRoot");
+      throw new NotInMountpointException(path, String.format("getEnclosingRoot - %s", ex.getMessage()));
     }
     Path mountPath = new Path(res.resolvedPath);
     Path enclosingPath = res.targetFileSystem.getEnclosingRoot(new Path(getUriPath(path)));
@@ -1940,11 +1940,13 @@ public Path getEnclosingRoot(Path path) throws IOException {
       try {
         res = fsState.resolve((path.toString()), true);
       } catch (FileNotFoundException ex) {
-        throw new NotInMountpointException(path, "getEnclosingRoot");
+        throw new NotInMountpointException(path, String.format("getEnclosingRoot - %s", ex.getMessage()));
       }
       Path fullPath = new Path(res.resolvedPath);
       Path enclosingPath = res.targetFileSystem.getEnclosingRoot(path);
-      return enclosingPath.depth() > fullPath.depth() ?  enclosingPath : fullPath;
+      return enclosingPath.depth() > fullPath.depth()
+          ?  enclosingPath
+          : fullPath;
     }
   }
 

diff --git a/...common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestGetEnclosingRoot.java b/...common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestGetEnclosingRoot.java
@@ -21,25 +21,24 @@
 import java.security.PrivilegedExceptionAction;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.test.HadoopTestBase;
 import org.junit.Test;
 
-import static org.junit.Assert.*;
-
-
-public class TestGetEnclosingRoot {
+public class TestGetEnclosingRoot extends HadoopTestBase {
   @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(root, fs.getEnclosingRoot(root));
+    assertEquals(root, fs.getEnclosingRoot(foobar));
+    assertEquals(root, fs.getEnclosingRoot(fs.getEnclosingRoot(foobar)));
+    assertEquals(fs.getEnclosingRoot(root), fs.getEnclosingRoot(foobar));
 
-    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));
+    assertEquals(root, fs.getEnclosingRoot(path(foobar.toString())));
+    assertEquals(root, fs.getEnclosingRoot(fs.getEnclosingRoot(path(foobar.toString()))));
+    assertEquals(fs.getEnclosingRoot(root), fs.getEnclosingRoot(path(foobar.toString())));
   }
 
   @Test
@@ -49,8 +48,8 @@ public void testEnclosingRootPathExists() throws Exception {
     Path foobar = path("/foo/bar");
     fs.mkdirs(foobar);
 
-    assertEquals(fs.getEnclosingRoot(foobar), root);
-    assertEquals(fs.getEnclosingRoot(path(foobar.toString())), root);
+    assertEquals(root, fs.getEnclosingRoot(foobar));
+    assertEquals(root, fs.getEnclosingRoot(path(foobar.toString())));
   }
 
   @Test
@@ -59,23 +58,23 @@ public void testEnclosingRootPathDNE() throws Exception {
     Path foobar = path("/foo/bar");
     Path root = path("/");
 
-    assertEquals(fs.getEnclosingRoot(foobar), root);
-    assertEquals(fs.getEnclosingRoot(path(foobar.toString())), root);
+    assertEquals(root, fs.getEnclosingRoot(foobar));
+    assertEquals(root, fs.getEnclosingRoot(path(foobar.toString())));
   }
 
   @Test
   public void testEnclosingRootWrapped() throws Exception {
     FileSystem fs = getFileSystem();
     Path root = path("/");
 
-    assertEquals(fs.getEnclosingRoot(new Path("/foo/bar")), root);
+    assertEquals(root, fs.getEnclosingRoot(new Path("/foo/bar")));
 
     UserGroupInformation ugi = UserGroupInformation.createRemoteUser("foo");
     Path p = ugi.doAs((PrivilegedExceptionAction<Path>) () -> {
       FileSystem wFs = getFileSystem();
       return wFs.getEnclosingRoot(new Path("/foo/bar"));
     });
-    assertEquals(p, root);
+    assertEquals(root, p);
   }
 
   private FileSystem getFileSystem() throws IOException {

diff --git a/...-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java b/...-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java
@@ -30,24 +30,19 @@
 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(root, fs.getEnclosingRoot(foobar));
+    assertEquals(root, fs.getEnclosingRoot(fs.getEnclosingRoot(foobar)));
+    assertEquals(fs.getEnclosingRoot(root), fs.getEnclosingRoot(foobar));
 
-    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));
+    assertEquals(root, fs.getEnclosingRoot(path(foobar.toString())));
+    assertEquals(root, fs.getEnclosingRoot(fs.getEnclosingRoot(path(foobar.toString()))));
+    assertEquals(fs.getEnclosingRoot(root), fs.getEnclosingRoot(path(foobar.toString())));
   }
 
   @Test
@@ -57,8 +52,8 @@ public void testEnclosingRootPathExists() throws Exception {
     Path foobar = path("/foo/bar");
     fs.mkdirs(foobar);
 
-    assertEquals(fs.getEnclosingRoot(foobar), root);
-    assertEquals(fs.getEnclosingRoot(path(foobar.toString())), root);
+    assertEquals(root, fs.getEnclosingRoot(foobar));
+    assertEquals(root, fs.getEnclosingRoot(path(foobar.toString())));
   }
 
   @Test
@@ -67,23 +62,23 @@ public void testEnclosingRootPathDNE() throws Exception {
     Path foobar = path("/foo/bar");
     Path root = path("/");
 
-    assertEquals(fs.getEnclosingRoot(foobar), root);
-    assertEquals(fs.getEnclosingRoot(path(foobar.toString())), root);
+    assertEquals(root, fs.getEnclosingRoot(foobar));
+    assertEquals(root, fs.getEnclosingRoot(path(foobar.toString())));
   }
 
   @Test
   public void testEnclosingRootWrapped() throws Exception {
     FileSystem fs = getFileSystem();
     Path root = path("/");
 
-    assertEquals(fs.getEnclosingRoot(new Path("/foo/bar")), root);
+    assertEquals(root, fs.getEnclosingRoot(new Path("/foo/bar")));
 
     UserGroupInformation ugi = UserGroupInformation.createRemoteUser("foo");
     Path p = ugi.doAs((PrivilegedExceptionAction<Path>) () -> {
       FileSystem wFs = getContract().getTestFileSystem();
       return wFs.getEnclosingRoot(new Path("/foo/bar"));
     });
-    assertEquals(p, root);
+    assertEquals(root, p);
   }
 
   /**
@@ -97,4 +92,4 @@ protected Path path(String filepath) throws IOException {
     return getFileSystem().makeQualified(
         new Path(getContract().getTestPath(), filepath));
   }
-}
+}
diff --git a/...roject/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java b/...roject/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
@@ -3942,7 +3942,15 @@ public LocatedBlocks next(final FileSystem fs, final Path p)
     }.resolve(this, absF);
   }
 
-  /* HDFS only */
+  /**
+   * Return path of the enclosing root for a given path
+   * The enclosing root path is a common ancestor that should be used for temp and staging dirs
+   * as well as within encryption zones and other restricted directories.
+   *
+   * @param path file path to find the enclosing root path for
+   * @return a path to the enclosing root
+   * @throws IOException early checks like failure to resolve path cause IO failures
+   */
   public Path getEnclosingRoot(final Path path) throws IOException {
     statistics.incrementReadOps(1);
     storageStatistics.incrementOpCounter(OpType.GET_ENCLOSING_ROOT);

diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEnclosingRoot.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEnclosingRoot.java
@@ -111,29 +111,39 @@ public void testBasicOperations() throws Exception {
     final Path zone1 = new Path(rootDir, "zone1");
 
     // Ensure that the root "/" returns the root without mount points or encryption zones
-    assertThat(rootDir.equals(fs.getEnclosingRoot(rootDir)));
+    assertThat(fs.getEnclosingRoot(rootDir))
+        .describedAs("enclosing root of %s", rootDir)
+        .isEqualTo(rootDir);
 
     // Ensure a dir returns the root without mount points or encryption zones
-    assertThat(rootDir.equals(fs.getEnclosingRoot(zone1)));
+    assertThat(fs.getEnclosingRoot(zone1))
+        .describedAs("enclosing root of %s", zone1)
+        .isEqualTo(rootDir);
 
     // create an encryption zone
     fs.mkdirs(zone1);
     dfsAdmin.createEncryptionZone(zone1, TEST_KEY, NO_TRASH);
 
     // Ensure that the root "/" returns the root with an encryption zone present
-    assertThat(rootDir.equals(fs.getEnclosingRoot(rootDir)));
+    assertThat(fs.getEnclosingRoot(rootDir))
+        .describedAs("enclosing root of %s", rootDir)
+        .isEqualTo(rootDir);
 
     // Ensure that the encryption zone path itself returns correctly as itself
-    assertThat(zone1.equals(fs.getEnclosingRoot(zone1)));
+    assertThat(fs.getEnclosingRoot(zone1))
+        .describedAs("enclosing root of %s", zone1)
+        .isEqualTo(zone1);
 
     // Ensure that a path where the file does not exist returns the encryption zone root path
     final Path zone1FileDNE = new Path(zone1, "newDNE.txt");
-    assertThat(zone1.equals(fs.getEnclosingRoot(zone1FileDNE)));
+    assertThat(fs.getEnclosingRoot(zone1FileDNE))
+        .describedAs("enclosing root of %s", zone1FileDNE)
+        .isEqualTo(zone1);
 
     // Ensure that a path where the dir does not exist returns the encryption zone root path
     final Path zone1DirDNE = new Path(zone1, "zone2/newDNE.txt");
-    assertThat(zone1.equals(fs.getEnclosingRoot(zone1DirDNE)));
-
+    assertThat(fs.getEnclosingRoot(zone1DirDNE))
+        .describedAs("enclosing root of %s", zone1DirDNE)
+        .isEqualTo(zone1);
   }
-
 }