From 5b68358d383ffd024885b60a4db6b4ca4be9dd82 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 22 Jun 2021 17:04:32 +0800 Subject: [PATCH 1/2] Remote: Do not upload empty output to remote cache. --- .../build/lib/remote/RemoteCache.java | 30 ++++++++++++- .../build/lib/remote/RemoteCacheTests.java | 44 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index 2ac802907e6fbb..b6feab04ddb498 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -132,6 +132,32 @@ public ActionResult downloadActionResult( return getFromFuture(cacheProtocol.downloadActionResult(context, actionKey, inlineOutErr)); } + /** Upload a local file to the remote cache. */ + public ListenableFuture uploadFile( + RemoteActionExecutionContext context, + Digest digest, + Path file + ) { + if (digest.getSizeBytes() == 0) { + return COMPLETED_SUCCESS; + } + + return cacheProtocol.uploadFile(context, digest, file); + } + + /** Upload sequence of bytes to the remote cache. */ + public ListenableFuture uploadBlob( + RemoteActionExecutionContext context, + Digest digest, + ByteString data + ) { + if (digest.getSizeBytes() == 0) { + return COMPLETED_SUCCESS; + } + + return cacheProtocol.uploadBlob(context, digest, data); + } + /** * Upload the result of a locally executed action to the remote cache. * @@ -212,14 +238,14 @@ private void uploadOutputs( for (Digest digest : digestsToUpload) { Path file = digestToFile.get(digest); if (file != null) { - uploads.add(cacheProtocol.uploadFile(context, digest, file)); + uploads.add(uploadFile(context, digest, file)); } else { ByteString blob = digestToBlobs.get(digest); if (blob == null) { String message = "FindMissingBlobs call returned an unknown digest: " + digest; throw new IOException(message); } - uploads.add(cacheProtocol.uploadBlob(context, digest, blob)); + uploads.add(uploadBlob(context, digest, blob)); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java index e4551c7d969b23..063358c56957f7 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java @@ -1547,6 +1547,50 @@ public void testUploadDirectory() throws Exception { assertThat(remoteCache.findMissingDigests(context, toQuery)).isEmpty(); } + @Test + public void testUploadEmptyBlobAndFile() throws Exception { + // Test that uploading an empty BLOB/file does not try to perform an upload. + InMemoryRemoteCache remoteCache = newRemoteCache(); + Digest emptyDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("file"), ""); + Path file = execRoot.getRelative("file"); + + Utils.getFromFuture(remoteCache.uploadBlob(context, emptyDigest, ByteString.EMPTY)); + assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest))) + .containsExactly(emptyDigest); + + Utils.getFromFuture(remoteCache.uploadFile(context, emptyDigest, file)); + assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest))) + .containsExactly(emptyDigest); + } + + @Test + public void testUploadEmptyOutputs() throws Exception { + // Test that uploading an empty output does not try to perform an upload. + + // arrange + Digest emptyDigest = + fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/test/wobble"), ""); + Path file = execRoot.getRelative("bar/test/wobble"); + InMemoryRemoteCache remoteCache = newRemoteCache(); + Action action = Action.getDefaultInstance(); + ActionKey actionDigest = digestUtil.computeActionKey(action); + Command cmd = Command.getDefaultInstance(); + + // act + remoteCache.upload( + context, + remotePathResolver, + actionDigest, + action, + cmd, + ImmutableList.of(file), + new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))); + + // assert + assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest))) + .containsExactly(emptyDigest); + } + @Test public void testUploadEmptyDirectory() throws Exception { // Test that uploading an empty directory works. From ff1cfaf8486baeb5c6c5f15a8a4abe4f5aabf210 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 22 Jun 2021 18:18:38 +0800 Subject: [PATCH 2/2] Fix tests. --- .../build/lib/remote/RemoteCache.java | 14 ++++++++++--- .../build/lib/remote/RemoteCacheTests.java | 21 +++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index b6feab04ddb498..ecc3b64d68cf1f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -614,7 +614,15 @@ public void onFailure(Throwable t) { return outerF; } - private List> downloadOutErr( + private ListenableFuture downloadBlob(RemoteActionExecutionContext context, Digest digest, OutputStream out) { + if (digest.getSizeBytes() == 0) { + return COMPLETED_SUCCESS; + } + + return cacheProtocol.downloadBlob(context, digest, out); + } + + public List> downloadOutErr( RemoteActionExecutionContext context, ActionResult result, OutErr outErr) { List> downloads = new ArrayList<>(); if (!result.getStdoutRaw().isEmpty()) { @@ -627,7 +635,7 @@ private List> downloadOutErr( } else if (result.hasStdoutDigest()) { downloads.add( Futures.transform( - cacheProtocol.downloadBlob( + downloadBlob( context, result.getStdoutDigest(), outErr.getOutputStream()), (d) -> null, directExecutor())); @@ -642,7 +650,7 @@ private List> downloadOutErr( } else if (result.hasStderrDigest()) { downloads.add( Futures.transform( - cacheProtocol.downloadBlob( + downloadBlob( context, result.getStderrDigest(), outErr.getErrorStream()), (d) -> null, directExecutor())); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java index 063358c56957f7..9da0741839dded 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java @@ -1304,6 +1304,27 @@ public void testDownloadEmptyBlobAndFile() throws Exception { assertThat(file.getFileSize()).isEqualTo(0); } + @Test + public void testDownloadEmptyOutErr() throws Exception { + // Test that downloading empty stdout/stderr does not try to perform a download. + + InMemoryRemoteCache remoteCache = newRemoteCache(); + Digest emptyDigest = digestUtil.compute(new byte[0]); + ActionResult.Builder result = ActionResult.newBuilder(); + result.setStdoutDigest(emptyDigest); + result.setStderrDigest(emptyDigest); + + RemoteCache.waitForBulkTransfer( + remoteCache.downloadOutErr( + context, + result.build(), + new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))), + true); + + assertThat(remoteCache.getNumSuccessfulDownloads()).isEqualTo(0); + assertThat(remoteCache.getNumFailedDownloads()).isEqualTo(0); + } + @Test public void testDownloadFileWithSymlinkTemplate() throws Exception { // Test that when a symlink template is provided, we don't actually download files to disk.