[
https://issues.apache.org/jira/browse/HDFS-16791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17691656#comment-17691656
]
ASF GitHub Bot commented on HDFS-16791:
---------------------------------------
steveloughran commented on code in PR #4967:
URL: https://github.com/apache/hadoop/pull/4967#discussion_r1113169004
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java:
##########
@@ -1638,6 +1638,24 @@ public MultipartUploaderBuilder
createMultipartUploader(Path basePath)
return null;
}
+ /**
+ * 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.
+ *
+ * Call makeQualified on the param path to ensure the param path to ensure
its part of the correct filesystem
+ *
+ * @param path file path to find the enclosing root path for
+ * @return a path to the enclosing root
+ * @throws IOException
+ */
+ @InterfaceAudience.Public
+ @InterfaceStability.Unstable
+ public Path getEnclosingRoot(Path path) throws IOException {
+ this.makeQualified(path);
Review Comment:
no need for `this.` prefix
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java:
##########
@@ -1370,6 +1370,20 @@ public boolean hasPathCapability(Path path, String
capability)
}
}
+ @Override
+ public Path getEnclosingRoot(Path path) throws IOException {
+ InodeTree.ResolveResult<FileSystem> res;
+ try {
+ res = fsState.resolve(getUriPath(path), true);
+ } catch (FileNotFoundException ex) {
+ throw new NotInMountpointException(path, "getEnclosingRoot");
Review Comment:
include inner exception as cause
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java:
##########
@@ -4904,6 +4904,24 @@ public CompletableFuture<FSDataInputStream> build()
throws IOException {
}
+ /**
+ * 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.
+ *
+ * Call makeQualified on the param path to ensure the param path to ensure
its part of the correct filesystem
+ *
+ * @param path file path to find the enclosing root path for
+ * @return a path to the enclosing root
+ * @throws IOException
Review Comment:
nit: add something here, even if just "IO failure". the more detail the
better
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestGetEnclosingRoot.java:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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;
+
+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+
+public class TestGetEnclosingRoot {
Review Comment:
should extend HadoopTestBase for timeouts, thread naming.
##########
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);
Review Comment:
again, all the assertions are inverted.
##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java:
##########
@@ -3941,4 +3941,23 @@ public LocatedBlocks next(final FileSystem fs, final
Path p)
}
}.resolve(this, absF);
}
+
+ /* HDFS only */
Review Comment:
use javadocs please
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java:
##########
@@ -1919,6 +1933,19 @@ public Collection<? extends BlockStoragePolicySpi>
getAllStoragePolicies()
}
return allPolicies;
}
+
+ @Override
+ public Path getEnclosingRoot(Path path) throws IOException {
+ InodeTree.ResolveResult<FileSystem> 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:
style nit: put the ? and the : on new lines to make clear they are special
##########
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 {
+ FileSystem fs = getFileSystem();
+ Path root = path("/");
+ Path foobar = path("/foo/bar");
+ fs.mkdirs(foobar);
+
+ assertEquals(fs.getEnclosingRoot(foobar), root);
+ assertEquals(fs.getEnclosingRoot(path(foobar.toString())), root);
+ }
+
+ @Test
+ public void testEnclosingRootPathDNE() throws Exception {
+ FileSystem fs = getFileSystem();
+ Path foobar = path("/foo/bar");
+ Path root = path("/");
+
+ assertEquals(fs.getEnclosingRoot(foobar), root);
+ assertEquals(fs.getEnclosingRoot(path(foobar.toString())), root);
+ }
+
+ @Test
+ public void testEnclosingRootWrapped() throws Exception {
+ FileSystem fs = getFileSystem();
+ Path root = path("/");
+
+ assertEquals(fs.getEnclosingRoot(new Path("/foo/bar")), root);
+
+ 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);
+ }
+
+ /**
+ * Create a path under the test path provided by
+ * the FS contract.
+ * @param filepath path string in
+ * @return a path qualified by the test filesystem
+ * @throws IOException IO problems
+ */
+ protected Path path(String filepath) throws IOException {
+ return getFileSystem().makeQualified(
+ new Path(getContract().getTestPath(), filepath));
+ }
+}
Review Comment:
nit, add a newline at EOF
##########
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<AbstractFileSystem> res;
+ try {
+ res = fsState.resolve((path.toString()), true);
+ } catch (FileNotFoundException ex) {
+ throw new NotInMountpointException(path, "getEnclosingRoot");
Review Comment:
I'd like it included. just because the current code doesn't do deep
reporting, doesn't mean new code should retain the bad habit.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java:
##########
@@ -4904,6 +4904,24 @@ public CompletableFuture<FSDataInputStream> build()
throws IOException {
}
+ /**
+ * 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.
+ *
+ * Call makeQualified on the param path to ensure the param path to ensure
its part of the correct filesystem
+ *
+ * @param path file path to find the enclosing root path for
+ * @return a path to the enclosing root
+ * @throws IOException
+ */
+ @InterfaceAudience.Public
+ @InterfaceStability.Unstable
+ public Path getEnclosingRoot(Path path) throws IOException {
+ this.makeQualified(path);
Review Comment:
again, `this.` superfluous
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEnclosingRoot.java:
##########
@@ -0,0 +1,139 @@
+/**
+ * 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.hdfs;
+
+import java.io.File;
+import java.util.EnumSet;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.crypto.key.JavaKeyStoreProvider;
+import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
+import org.apache.hadoop.fs.FileSystemTestHelper;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hdfs.client.CreateEncryptionZoneFlag;
+import org.apache.hadoop.hdfs.client.HdfsAdmin;
+import org.apache.hadoop.hdfs.server.namenode.EncryptionFaultInjector;
+import org.apache.hadoop.hdfs.server.namenode.EncryptionZoneManager;
+import org.apache.hadoop.test.AbstractHadoopTestBase;
+import org.apache.hadoop.test.GenericTestUtils;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.event.Level;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class TestEnclosingRoot extends AbstractHadoopTestBase {
+ private static final Logger LOG =
LoggerFactory.getLogger(TestEnclosingRoot.class);
+ private static final String TEST_KEY = "test_key";
+ private static final EnumSet<CreateEncryptionZoneFlag> NO_TRASH =
+ EnumSet.of(CreateEncryptionZoneFlag.NO_TRASH);
+
+ private Configuration conf;
+ private FileSystemTestHelper fsHelper;
+
+ private MiniDFSCluster cluster;
+ private HdfsAdmin dfsAdmin;
+ private DistributedFileSystem fs;
+ private File testRootDir;
+
+ private String getKeyProviderURI() {
+ return JavaKeyStoreProvider.SCHEME_NAME + "://file" +
+ new Path(testRootDir.toString(), "test.jks").toUri();
+ }
+
+ @Before
+ public void setup() throws Exception {
+ conf = new HdfsConfiguration();
+ fsHelper = new FileSystemTestHelper();
+ // Set up java key store
+ String testRoot = fsHelper.getTestRootDir();
+ testRootDir = new File(testRoot).getAbsoluteFile();
+ conf.set(CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH,
+ getKeyProviderURI());
+
conf.setBoolean(DFSConfigKeys.DFS_NAMENODE_DELEGATION_TOKEN_ALWAYS_USE_KEY,
true);
+ // Lower the batch size for testing
+ conf.setInt(DFSConfigKeys.DFS_NAMENODE_LIST_ENCRYPTION_ZONES_NUM_RESPONSES,
+ 2);
+ cluster = new MiniDFSCluster.Builder(conf).numDataNodes(1).build();
+ cluster.waitActive();
+ GenericTestUtils.setLogLevel(
+ LoggerFactory.getLogger(EncryptionZoneManager.class), Level.TRACE);
+ fs = cluster.getFileSystem();
+ dfsAdmin = new HdfsAdmin(cluster.getURI(), conf);
+ setProvider();
+ // Create a test key
+ DFSTestUtil.createKey(TEST_KEY, cluster, conf);
+ }
+
+ protected void setProvider() {
+ // Need to set the client's KeyProvider to the NN's for JKS,
+ // else the updates do not get flushed properly
+ fs.getClient().setKeyProvider(cluster.getNameNode().getNamesystem()
+ .getProvider());
+ }
+
+ @After
+ public void teardown() {
+ try {
+ if (cluster != null) {
+ cluster.shutdown();
+ cluster = null;
+ }
+ } finally {
+ EncryptionFaultInjector.instance = new EncryptionFaultInjector();
+ }
+ }
+
+ @Test
+ /**
+ * Testing basic operations for getEnclosingRoot with
dfs/DistributedFileSystem
+ */
+ public void testBasicOperations() throws Exception {
+ final Path rootDir = new Path("/");
+ 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)));
Review Comment:
the better assertj style is
```
AssertJ.assertThat(fs.getEnclosingRoot(rootDir)
.describedAs("enclosing root of %s", rootDir)
.isEqualTo(rootDir);
```
please use here and below...the more diagnostics on an assertion failure,
the better
> New API for enclosing root path for a file
> ------------------------------------------
>
> Key: HDFS-16791
> URL: https://issues.apache.org/jira/browse/HDFS-16791
> Project: Hadoop HDFS
> Issue Type: Improvement
> Reporter: Tom McCormick
> Assignee: Tom McCormick
> Priority: Major
> Labels: pull-request-available
>
> At LinkedIn we run many HDFS volumes that are federated by either
> ViewFilesystem or Router Based Federation. As our number of hdfs volumes
> grows, we have a growing need to migrate data seemlessly across volumes.
> Many frameworks have a notion of staging or temp directories, but those
> directories often live in random locations. We want an API getEnclosingRoot,
> which provides the root path a file or dataset.
> In ViewFilesystem / Router Based Federation, the enclosingRoot will be the
> mount point.
> We will also take into account other restrictions for renames like
> encryptions zones.
> If there are several paths (a mount point and an encryption zone), we will
> return the longer path
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
