Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.1.0] Avoid exception-based control flow in RAFS#getDigest and RAFS#getFastDigest. #21264

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -90,21 +90,21 @@ public class RemoteActionFileSystem extends AbstractFileSystemWithCustomStat {

@Nullable private ActionExecutionMetadata action = null;

/** Describes how to handle symlinks when calling {@link #statUnchecked}. */
/** Describes how to handle symlinks when calling {@link #statInternal}. */
private enum FollowMode {
/** Dereference the entire path. This is equivalent to {@link Symlinks.FOLLOW}. */
/** Canonicalize the entire path. This is equivalent to {@link Symlinks.FOLLOW}. */
FOLLOW_ALL,
/** Dereference the parent path. This is equivalent to {@link Symlinks.NOFOLLOW}. */
/** Canonicalize the parent path. This is equivalent to {@link Symlinks.NOFOLLOW}. */
FOLLOW_PARENT,
/** Do not dereference. This is only used internally to resolve symlinks efficiently. */
/** Do not canonicalize. This is only used internally to resolve symlinks efficiently. */
FOLLOW_NONE
};

/** Describes which sources to consider when statting a file. */
/** Describes which sources to consider when calling {@link #statInternal}. */
private enum StatSources {
/** Consider all sources (action input map, in-memory filesystem and local filesystem). */
/** Consider all sources (action input map, remote output tree and local filesystem). */
ALL,
/** Consider only in-memory sources (action input map and in-memory filesystem). */
/** Consider only in-memory sources (action input map and remote output tree). */
IN_MEMORY_ONLY,
}

Expand Down Expand Up @@ -267,13 +267,9 @@ boolean isRemote(Path path) throws IOException {

private boolean isRemote(PathFragment path) throws IOException {
// Files in the local filesystem are non-remote by definition, so stat only in-memory sources.
try {
var status = statUnchecked(path, FollowMode.FOLLOW_ALL, StatSources.IN_MEMORY_ONLY);
return (status instanceof FileStatusWithMetadata)
&& ((FileStatusWithMetadata) status).getMetadata().isRemote();
} catch (FileNotFoundException e) {
return false;
}
var status = statInternal(path, FollowMode.FOLLOW_ALL, StatSources.IN_MEMORY_ONLY);
return (status instanceof FileStatusWithMetadata)
&& ((FileStatusWithMetadata) status).getMetadata().isRemote();
}

public void updateContext(ActionExecutionMetadata action) {
Expand Down Expand Up @@ -365,18 +361,15 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks)
}

@Override
@Nullable
protected byte[] getFastDigest(PathFragment path) throws IOException {
path = resolveSymbolicLinks(path).asFragment();
// Try to obtain a fast digest through a stat. This is only possible for in-memory files.
// The parent path has already been canonicalized by resolveSymbolicLinks, so FOLLOW_NONE is
// effectively the same as FOLLOW_PARENT, but more efficient.
try {
var status = statUnchecked(path, FollowMode.FOLLOW_NONE, StatSources.IN_MEMORY_ONLY);
if (status instanceof FileStatusWithDigest) {
return ((FileStatusWithDigest) status).getDigest();
}
} catch (FileNotFoundException e) {
// Intentionally ignored.
var status = statInternal(path, FollowMode.FOLLOW_NONE, StatSources.IN_MEMORY_ONLY);
if (status instanceof FileStatusWithDigest) {
return ((FileStatusWithDigest) status).getDigest();
}
return localFs.getPath(path).getFastDigest();
}
Expand All @@ -387,13 +380,9 @@ protected byte[] getDigest(PathFragment path) throws IOException {
// Try to obtain a fast digest through a stat. This is only possible for in-memory files.
// The parent path has already been canonicalized by resolveSymbolicLinks, so FOLLOW_NONE is
// effectively the same as FOLLOW_PARENT, but more efficient.
try {
var status = statUnchecked(path, FollowMode.FOLLOW_NONE, StatSources.IN_MEMORY_ONLY);
if (status instanceof FileStatusWithDigest) {
return ((FileStatusWithDigest) status).getDigest();
}
} catch (FileNotFoundException e) {
// Intentionally ignored.
var status = statInternal(path, FollowMode.FOLLOW_NONE, StatSources.IN_MEMORY_ONLY);
if (status instanceof FileStatusWithDigest) {
return ((FileStatusWithDigest) status).getDigest();
}
return localFs.getPath(path).getDigest();
}
Expand Down Expand Up @@ -529,7 +518,10 @@ protected PathFragment resolveOneLink(PathFragment path) throws IOException {
//
// The parent path has already been canonicalized, so FOLLOW_NONE is effectively the same as
// FOLLOW_PARENT, but much more efficient as it doesn't call stat recursively.
var stat = statUnchecked(path, FollowMode.FOLLOW_NONE, StatSources.ALL);
var stat = statInternal(path, FollowMode.FOLLOW_NONE, StatSources.ALL);
if (stat == null) {
throw new FileNotFoundException(path.getPathString() + " (No such file or directory)");
}
return stat.isSymbolicLink() ? readSymbolicLink(path) : null;
}

Expand All @@ -554,34 +546,45 @@ protected boolean exists(PathFragment path, boolean followSymlinks) {
}
}

@Override
protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException {
FileStatus stat = statIfFound(path, followSymlinks);
if (stat == null) {
throw new FileNotFoundException(path.getPathString() + " (No such file or directory)");
}
return stat;
}

@Nullable
@Override
protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException {
try {
return stat(path, followSymlinks);
} catch (FileNotFoundException e) {
return null;
}
return statInternal(
path, followSymlinks ? FollowMode.FOLLOW_ALL : FollowMode.FOLLOW_PARENT, StatSources.ALL);
}

@Nullable
@Override
protected FileStatus statNullable(PathFragment path, boolean followSymlinks) {
try {
return stat(path, followSymlinks);
return statIfFound(path, followSymlinks);
} catch (IOException e) {
return null;
}
}

@Override
protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException {
return statUnchecked(
path, followSymlinks ? FollowMode.FOLLOW_ALL : FollowMode.FOLLOW_PARENT, StatSources.ALL);
}

private FileStatus statUnchecked(
PathFragment path, FollowMode followMode, StatSources statSources) throws IOException {
/**
* Internal stat implementation.
*
* @param path the path to stat
* @param followMode whether and how to canonicalize the path
* @param statSources which sources to consider
* @return the file status on success, or null if the file was not found in any of the sources
* under consideration
* @throws IOException if an error other than file not found occurred
*/
@Nullable
private FileStatus statInternal(PathFragment path, FollowMode followMode, StatSources statSources)
throws IOException {
// Canonicalize the path.
if (followMode == FollowMode.FOLLOW_ALL) {
path = resolveSymbolicLinks(path).asFragment();
Expand Down Expand Up @@ -611,10 +614,10 @@ private FileStatus statUnchecked(
}

if (statSources == StatSources.ALL) {
return localFs.getPath(path).stat(Symlinks.NOFOLLOW);
return localFs.getPath(path).statIfFound(Symlinks.NOFOLLOW);
}

throw new FileNotFoundException(path.getPathString() + " (No such file or directory)");
return null;
}

private static FileStatusWithMetadata statFromMetadata(FileArtifactValue m) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks)
* filesystem doesn't support them. This digest should be suitable for detecting changes to the
* file.
*/
@Nullable
protected byte[] getFastDigest(PathFragment path) throws IOException {
return null;
}
Expand Down
Loading