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.

Sign up for GitHub

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

Jump to bottom

[Remote Cache] Don't delete tree artifacts on failure #6851

Closed
wants to merge 3 commits into from
Closed

[Remote Cache] Don't delete tree artifacts on failure #6851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7417c1
[Remote Cache] Don't delete tree artifacts on failure
keith Dec 5, 2018
3a591e4
Add comment
keith Dec 5, 2018
56ed7ab
Add test
keith Dec 6, 2018
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
4 changes: 3 additions & 1 deletion src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,9 @@ public void onFailure(Throwable t) {
execRoot.getRelative(file.getPath()).delete();
}
for (OutputDirectory directory : result.getOutputDirectoriesList()) {
FileSystemUtils.deleteTree(execRoot.getRelative(directory.getPath()));
// Only delete the directories below the output directories because the output
// directories will not be re-created
FileSystemUtils.deleteTreesBelow(execRoot.getRelative(directory.getPath()));
keith marked this conversation as resolved.
Show resolved Hide resolved
keith marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just had a thought, this method's name and documentation implies that it deletes only the subdirectories inside the given directory, but there may also be regular files rooted at this directory. @buchgr can you confirm that it deletes the files as well? I traced through the code, but wasn't sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there ever be regular files that weren't part of the output? The output ones are deleted above, but otherwise I can stop using this helper and directly loop over the children of this directory

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be to leave the original line, and call createDirectoryAndParents afterwards

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let @buchgr decide what's best.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line looks good to me. It deletes everything in a directory, including files symlinks and subdirectories. I agree that the naming could be improved.

}
if (outErr != null) {
outErr.getOutputPath().delete();
Expand Down
25 changes: 25 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,31 @@ public void downloadAbsoluteSymlinkInDirectoryError() throws Exception {
}
}

@Test
public void downloadFailureMaintainsDirectories() throws Exception {
DefaultRemoteActionCache cache = newTestCache();
Tree tree = Tree.newBuilder().setRoot(Directory.newBuilder()).build();
Digest treeDigest = cache.addContents(tree.toByteArray());
Digest outputFileDigest = cache
.addException("outputdir/outputfile", new IOException("download failed"));
Digest otherFileDigest = cache.addContents("otherfile");

ActionResult.Builder result = ActionResult.newBuilder();
result.addOutputDirectoriesBuilder().setPath("outputdir").setTreeDigest(treeDigest);
result.addOutputFiles(
OutputFile.newBuilder().setPath("outputdir/outputfile").setDigest(outputFileDigest));
result.addOutputFiles(OutputFile.newBuilder().setPath("otherfile").setDigest(otherFileDigest));
try {
cache.download(result.build(), execRoot, null);
fail("Expected exception");
} catch (IOException expected) {
assertThat(cache.getNumFailedDownloads()).isEqualTo(1);
assertThat(execRoot.getRelative("outputdir").exists()).isTrue();
assertThat(execRoot.getRelative("outputdir/outputfile").exists()).isFalse();
assertThat(execRoot.getRelative("otherfile").exists()).isFalse();
}
}

@Test
public void onErrorWaitForRemainingDownloadsToComplete() throws Exception {
// If one or more downloads of output files / directories fail then the code should
Expand Down