From 7f7bdbd5e7ee477e625c9818c1e97d8e5e169bd1 Mon Sep 17 00:00:00 2001 From: Tom McCormick Date: Wed, 11 Oct 2023 13:24:06 -0700 Subject: [PATCH] fixing pr comments --- .../apache/hadoop/fs/AbstractFileSystem.java | 4 +-- .../java/org/apache/hadoop/fs/FileSystem.java | 2 +- .../hadoop/fs/viewfs/ViewFileSystem.java | 8 +++-- .../hadoop/fs/TestGetEnclosingRoot.java | 31 +++++++++---------- .../AbstractContractGetEnclosingRoot.java | 31 ++++++++----------- .../hadoop/hdfs/DistributedFileSystem.java | 10 +++++- .../apache/hadoop/hdfs/TestEnclosingRoot.java | 26 +++++++++++----- 7 files changed, 63 insertions(+), 49 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java index da3bd0b71e26d..25e1f15264089 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java +++ b/hadoop-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 index 2bb9225d23755..063cfb76f32cd 100644 --- 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 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/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java index 663bb72f4a11e..c070812597cd6 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java +++ b/hadoop-common-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/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestGetEnclosingRoot.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestGetEnclosingRoot.java index c3c5b6cf6c446..8bbab36d53096 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestGetEnclosingRoot.java +++ b/hadoop-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,8 +58,8 @@ 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 @@ -68,14 +67,14 @@ 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) () -> { FileSystem wFs = getFileSystem(); return wFs.getEnclosingRoot(new Path("/foo/bar")); }); - assertEquals(p, root); + assertEquals(root, p); } private FileSystem getFileSystem() throws IOException { diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java index 02cdd302652d3..60c0632774b8f 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.java +++ b/hadoop-common-project/hadoop-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,8 +62,8 @@ 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 @@ -76,14 +71,14 @@ 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) () -> { 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)); } -} \ No newline at end of file +} diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java index b622ef867b0a4..63110dcb8f718 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java +++ b/hadoop-hdfs-project/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 index 3c40782ef22a8..85204e7a1a5b3 100644 --- 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); } - }