[ 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: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org