steveloughran commented on code in PR #6198: URL: https://github.com/apache/hadoop/pull/6198#discussion_r1366042141
########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.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.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); + + @Test + public void testEnclosingRootEquivalence() throws IOException { + FileSystem fs = getFileSystem(); + Path root = path("/"); + Path foobar = path("/foo/bar"); Review Comment: use methodPath() ... test contracts designed to run suites in parallel on the same fs need unique paths. I know you aren't creating any, but due diligence is good. ########## hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractGetEnclosingRoot.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.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); + + @Test + public void testEnclosingRootEquivalence() throws IOException { + FileSystem fs = getFileSystem(); + Path root = path("/"); + Path foobar = path("/foo/bar"); + + assertEquals(root, fs.getEnclosingRoot(foobar)); Review Comment: can add a description of each assert, so when we get a test failure we can see what assert failed without having to dive into the source ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java: ########## @@ -4944,6 +4944,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. + * Review Comment: nit: add a <p> for line breaks in javadocs and IDE rendering ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java: ########## @@ -1919,6 +1933,21 @@ 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, String.format("getEnclosingRoot - %s", ex.getMessage())); Review Comment: add the caught exception as the cause ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java: ########## @@ -1477,5 +1477,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: again, add the cause ########## hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md: ########## @@ -601,7 +601,32 @@ on the filesystem. 1. The outcome of this operation MUST be identical to the value of `getFileStatus(P).getBlockSize()`. -1. By inference, it MUST be > 0 for any file of length > 0. +2. By inference, it MUST be > 0 for any file of length > 0. + +### `Path getEnclosingRoot(Path p)` + +This method is used to find a root directory for a path given. This is useful for creating +staging and temp directories in the same enclosing root directory. There are constraints around how +renames are allowed to atomically occur (ex. across hdfs volumes or across encryption zones). + +For any two paths p1 and p2 that do not have the same enclosing root, `rename(p1, p2)` is expected to fail or will not Review Comment: ooh, nice point. Can you add. ``` Note that for object stores, even with the same enclosing root, there is no guarantee file or directory rename is atomic ``` ########## hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md: ########## @@ -601,7 +601,32 @@ on the filesystem. 1. The outcome of this operation MUST be identical to the value of `getFileStatus(P).getBlockSize()`. -1. By inference, it MUST be > 0 for any file of length > 0. +2. By inference, it MUST be > 0 for any file of length > 0. + +### `Path getEnclosingRoot(Path p)` + +This method is used to find a root directory for a path given. This is useful for creating +staging and temp directories in the same enclosing root directory. There are constraints around how +renames are allowed to atomically occur (ex. across hdfs volumes or across encryption zones). + +For any two paths p1 and p2 that do not have the same enclosing root, `rename(p1, p2)` is expected to fail or will not +be atomic. + +The following statement is always true: +`getEnclosingRoot(p) == getEnclosingRoot(getEnclosingRoot(p))` + +#### Preconditions + +The path does not have to exist, but the path does need to be valid and reconcilable by the filesystem +* if a linkfallback is used all paths are reconcilable +* if a linkfallback is not used there must be a mount point covering the path + + +#### Postconditions + +* The path returned will not be null, if there is no deeper enclosing root, the root path ("/") will be returned. +* The path returned is a directory Review Comment: can you add in the python-ish spec ```python path in ancestors(FS, p) or path == p isDir(FS, p) ``` ########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java: ########## @@ -4944,6 +4944,24 @@ public CompletableFuture<FSDataInputStream> build() throws IOException { } + /** + * Return path of the enclosing root for a given path Review Comment: add a "." to keep javadoc quiet -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
