Skip to content

Commit 18c8216

Browse files
coeuvrecopybara-github
coeuvre
authored and
copybara-github
committed
Remote: Do not upload empty output to remote cache.
An unintended side-effect of change cc2b3ec is that zero-sized blob won't be uploaded to gRPC cache. However, the behavior is existed for a long time and the REAPI spec is also updated accordingly. This change makes the behavior explicit and brings it to other remote cache backends. Context bazelbuild#11063. Fixes bazelbuild#13349. Closes bazelbuild#13594. PiperOrigin-RevId: 384457129
1 parent b27fd22 commit 18c8216

File tree

3 files changed

+119
-19
lines changed

3 files changed

+119
-19
lines changed

‎src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java‎

Lines changed: 53 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,38 @@ public ActionResult downloadActionResult(
139139
return getFromFuture(cacheProtocol.downloadActionResult(context, actionKey, inlineOutErr));
140140
}
141141

142+
/**
143+
* Upload a local file to the remote cache.
144+
*
145+
* @param context the context for the action.
146+
* @param digest the digest of the file.
147+
* @param file the file to upload.
148+
*/
149+
public final ListenableFuture<Void> uploadFile(
150+
RemoteActionExecutionContext context, Digest digest, Path file) {
151+
if (digest.getSizeBytes() == 0) {
152+
return COMPLETED_SUCCESS;
153+
}
154+
155+
return cacheProtocol.uploadFile(context, digest, file);
156+
}
157+
158+
/**
159+
* Upload sequence of bytes to the remote cache.
160+
*
161+
* @param context the context for the action.
162+
* @param digest the digest of the file.
163+
* @param data the BLOB to upload.
164+
*/
165+
public final ListenableFuture<Void> uploadBlob(
166+
RemoteActionExecutionContext context, Digest digest, ByteString data) {
167+
if (digest.getSizeBytes() == 0) {
168+
return COMPLETED_SUCCESS;
169+
}
170+
171+
return cacheProtocol.uploadBlob(context, digest, data);
172+
}
173+
142174
/**
143175
* Upload the result of a locally executed action to the remote cache.
144176
*
@@ -219,14 +251,14 @@ private void uploadOutputs(
219251
for (Digest digest : digestsToUpload) {
220252
Path file = digestToFile.get(digest);
221253
if (file != null) {
222-
uploads.add(cacheProtocol.uploadFile(context, digest, file));
254+
uploads.add(uploadFile(context, digest, file));
223255
} else {
224256
ByteString blob = digestToBlobs.get(digest);
225257
if (blob == null) {
226258
String message = "FindMissingBlobs call returned an unknown digest: " + digest;
227259
throw new IOException(message);
228260
}
229-
uploads.add(cacheProtocol.uploadBlob(context, digest, blob));
261+
uploads.add(uploadBlob(context, digest, blob));
230262
}
231263
}
232264

@@ -317,6 +349,15 @@ public void onFailure(Throwable t) {
317349
return outerF;
318350
}
319351

352+
private ListenableFuture<Void> downloadBlob(
353+
RemoteActionExecutionContext context, Digest digest, OutputStream out) {
354+
if (digest.getSizeBytes() == 0) {
355+
return COMPLETED_SUCCESS;
356+
}
357+
358+
return cacheProtocol.downloadBlob(context, digest, out);
359+
}
360+
320361
private static Path toTmpDownloadPath(Path actualPath) {
321362
return actualPath.getParentDirectory().getRelative(actualPath.getBaseName() + ".tmp");
322363
}
@@ -662,7 +703,14 @@ public void onFailure(Throwable t) {
662703
return outerF;
663704
}
664705

665-
private List<ListenableFuture<FileMetadata>> downloadOutErr(
706+
/**
707+
* Download the stdout and stderr of an executed action.
708+
*
709+
* @param context the context for the action.
710+
* @param result the result of the action.
711+
* @param outErr the {@link OutErr} that the stdout and stderr will be downloaded to.
712+
*/
713+
public final List<ListenableFuture<FileMetadata>> downloadOutErr(
666714
RemoteActionExecutionContext context, ActionResult result, OutErr outErr) {
667715
List<ListenableFuture<FileMetadata>> downloads = new ArrayList<>();
668716
if (!result.getStdoutRaw().isEmpty()) {
@@ -675,8 +723,7 @@ private List<ListenableFuture<FileMetadata>> downloadOutErr(
675723
} else if (result.hasStdoutDigest()) {
676724
downloads.add(
677725
Futures.transform(
678-
cacheProtocol.downloadBlob(
679-
context, result.getStdoutDigest(), outErr.getOutputStream()),
726+
downloadBlob(context, result.getStdoutDigest(), outErr.getOutputStream()),
680727
(d) -> null,
681728
directExecutor()));
682729
}
@@ -690,8 +737,7 @@ private List<ListenableFuture<FileMetadata>> downloadOutErr(
690737
} else if (result.hasStderrDigest()) {
691738
downloads.add(
692739
Futures.transform(
693-
cacheProtocol.downloadBlob(
694-
context, result.getStderrDigest(), outErr.getErrorStream()),
740+
downloadBlob(context, result.getStderrDigest(), outErr.getErrorStream()),
695741
(d) -> null,
696742
directExecutor()));
697743
}

‎src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -610,6 +610,7 @@ public void uploadSymlinkInDirectoryNoAllowError() throws Exception {
610610
assertThat(e).hasMessageThat().contains("--remote_allow_symlink_upload");
611611
}
612612

613+
// TODO(chiwang): Cleanup the tests, e.g. use java test data builder pattern.
613614
@Test
614615
public void downloadRelativeFileSymlink() throws Exception {
615616
RemoteCache cache = newRemoteCache();
@@ -1364,6 +1365,27 @@ public void testDownloadEmptyBlobAndFile() throws Exception {
13641365
assertThat(file.getFileSize()).isEqualTo(0);
13651366
}
13661367

1368+
@Test
1369+
public void downloadOutErr_empty_doNotPerformDownload() throws Exception {
1370+
// Test that downloading empty stdout/stderr does not try to perform a download.
1371+
1372+
InMemoryRemoteCache remoteCache = newRemoteCache();
1373+
Digest emptyDigest = digestUtil.compute(new byte[0]);
1374+
ActionResult.Builder result = ActionResult.newBuilder();
1375+
result.setStdoutDigest(emptyDigest);
1376+
result.setStderrDigest(emptyDigest);
1377+
1378+
RemoteCache.waitForBulkTransfer(
1379+
remoteCache.downloadOutErr(
1380+
context,
1381+
result.build(),
1382+
new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))),
1383+
true);
1384+
1385+
assertThat(remoteCache.getNumSuccessfulDownloads()).isEqualTo(0);
1386+
assertThat(remoteCache.getNumFailedDownloads()).isEqualTo(0);
1387+
}
1388+
13671389
@Test
13681390
public void testDownloadFileWithSymlinkTemplate() throws Exception {
13691391
// Test that when a symlink template is provided, we don't actually download files to disk.
@@ -1627,6 +1649,50 @@ public void testUploadDirectory() throws Exception {
16271649
assertThat(remoteCache.findMissingDigests(context, toQuery)).isEmpty();
16281650
}
16291651

1652+
@Test
1653+
public void upload_emptyBlobAndFile_doNotPerformUpload() throws Exception {
1654+
// Test that uploading an empty BLOB/file does not try to perform an upload.
1655+
InMemoryRemoteCache remoteCache = newRemoteCache();
1656+
Digest emptyDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("file"), "");
1657+
Path file = execRoot.getRelative("file");
1658+
1659+
Utils.getFromFuture(remoteCache.uploadBlob(context, emptyDigest, ByteString.EMPTY));
1660+
assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest)))
1661+
.containsExactly(emptyDigest);
1662+
1663+
Utils.getFromFuture(remoteCache.uploadFile(context, emptyDigest, file));
1664+
assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest)))
1665+
.containsExactly(emptyDigest);
1666+
}
1667+
1668+
@Test
1669+
public void upload_emptyOutputs_doNotPerformUpload() throws Exception {
1670+
// Test that uploading an empty output does not try to perform an upload.
1671+
1672+
// arrange
1673+
Digest emptyDigest =
1674+
fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/test/wobble"), "");
1675+
Path file = execRoot.getRelative("bar/test/wobble");
1676+
InMemoryRemoteCache remoteCache = newRemoteCache();
1677+
Action action = Action.getDefaultInstance();
1678+
ActionKey actionDigest = digestUtil.computeActionKey(action);
1679+
Command cmd = Command.getDefaultInstance();
1680+
1681+
// act
1682+
remoteCache.upload(
1683+
context,
1684+
remotePathResolver,
1685+
actionDigest,
1686+
action,
1687+
cmd,
1688+
ImmutableList.of(file),
1689+
new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr")));
1690+
1691+
// assert
1692+
assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest)))
1693+
.containsExactly(emptyDigest);
1694+
}
1695+
16301696
@Test
16311697
public void testUploadEmptyDirectory() throws Exception {
16321698
// Test that uploading an empty directory works.

‎src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import build.bazel.remote.execution.v2.Directory;
1919
import build.bazel.remote.execution.v2.DirectoryNode;
2020
import build.bazel.remote.execution.v2.FileNode;
21-
import com.google.common.util.concurrent.ListenableFuture;
2221
import com.google.devtools.build.lib.remote.RemoteCache;
2322
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
2423
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
@@ -27,7 +26,6 @@
2726
import com.google.devtools.build.lib.remote.util.DigestUtil;
2827
import com.google.devtools.build.lib.remote.util.Utils;
2928
import com.google.devtools.build.lib.vfs.Path;
30-
import com.google.protobuf.ByteString;
3129
import java.io.IOException;
3230

3331
/** A {@link RemoteCache} backed by an {@link DiskCacheClient}. */
@@ -61,16 +59,6 @@ public void downloadTree(
6159
}
6260
}
6361

64-
public ListenableFuture<Void> uploadFile(
65-
RemoteActionExecutionContext context, Digest digest, Path file) {
66-
return cacheProtocol.uploadFile(context, digest, file);
67-
}
68-
69-
public ListenableFuture<Void> uploadBlob(
70-
RemoteActionExecutionContext context, Digest digest, ByteString data) {
71-
return cacheProtocol.uploadBlob(context, digest, data);
72-
}
73-
7462
public void uploadActionResult(
7563
RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult)
7664
throws IOException, InterruptedException {

0 commit comments

Comments
 (0)