From 0a0d877a6960db1f4054f9b6ac09b6c91c311548 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 2 Sep 2024 03:24:23 -0700 Subject: [PATCH] Flip --remote_cache_async. Moves uploads to a disk or remote cache to the background by default. Some actions (specifically, those that modify spawn outputs after execution) are opted out, and still upload in a blocking manner. See ActionExecutionMetadata#mayModifySpawnOutputsAfterExecution and overrides. This is technically not an incompatible change because (in the absence of bugs) it doesn't affect user-visible behavior: asynchronous uploads cannot affect the behavior of hermetic actions. Fixes #21578. RELNOTES: Uploading local action results to a disk or remote cache now occurs in the background whenever possible, potentially unblocking the execution of followup actions. Set `--noremote_cache_async` to revert to the previous behavior. PiperOrigin-RevId: 670152543 Change-Id: Ibf6b36f723945b2534b61e228d440fe71a6019ac --- .../lib/remote/options/RemoteOptions.java | 11 +++++---- .../remote/RemoteExecutionServiceTest.java | 23 ++++++++++++------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 8f2acfbdfab344..c0b2b00b9fad65 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -102,13 +102,16 @@ public final class RemoteOptions extends CommonRemoteOptions { public PathFragment remoteCaptureCorruptedOutputs; @Option( - name = "experimental_remote_cache_async", - defaultValue = "false", + name = "remote_cache_async", + oldName = "experimental_remote_cache_async", + defaultValue = "true", documentationCategory = OptionDocumentationCategory.REMOTE, effectTags = {OptionEffectTag.UNKNOWN}, help = - "If true, remote cache I/O will happen in the background instead of taking place as the" - + " part of a spawn.") + "If true, uploading of action results to a disk or remote cache will happen in the" + + " background instead of blocking the completion of an action. Some actions are" + + " incompatible with background uploads, and may still block even when this flag is" + + " set.") public boolean remoteCacheAsync; @Option( diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index ea7d46ef90a81f..43197b883be99d 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -1650,7 +1650,7 @@ public void uploadOutputs_uploadDirectory_works() throws Exception { // act UploadManifest manifest = service.buildUploadManifest(action, spawnResult); - service.uploadOutputs(action, spawnResult, () -> {}); + uploadOutputsAndWait(service, action, spawnResult); // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -1697,7 +1697,7 @@ public void uploadOutputs_uploadEmptyDirectory_works() throws Exception { // act UploadManifest manifest = service.buildUploadManifest(action, spawnResult); - service.uploadOutputs(action, spawnResult, () -> {}); + uploadOutputsAndWait(service, action, spawnResult); // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -1771,7 +1771,7 @@ public void uploadOutputs_uploadNestedDirectory_works() throws Exception { // act UploadManifest manifest = service.buildUploadManifest(action, spawnResult); - service.uploadOutputs(action, spawnResult, () -> {}); + uploadOutputsAndWait(service, action, spawnResult); // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -1807,7 +1807,7 @@ private void doUploadDanglingSymlink(PathFragment targetPath) throws Exception { // act UploadManifest manifest = service.buildUploadManifest(action, spawnResult); - service.uploadOutputs(action, spawnResult, () -> {}); + uploadOutputsAndWait(service, action, spawnResult); // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -1859,7 +1859,7 @@ public void uploadOutputs_emptyOutputs_doNotPerformUpload() throws Exception { .build(); // act - service.uploadOutputs(action, spawnResult, () -> {}); + uploadOutputsAndWait(service, action, spawnResult); // assert assertThat( @@ -1885,7 +1885,7 @@ public void uploadOutputs_uploadFails_printWarning() throws Exception { .when(cache) .uploadActionResult(any(), any(), any()); - service.uploadOutputs(action, spawnResult, () -> {}); + uploadOutputsAndWait(service, action, spawnResult); assertThat(eventHandler.getEvents()).hasSize(1); Event evt = eventHandler.getEvents().get(0); @@ -1910,7 +1910,7 @@ public void uploadOutputs_firesUploadEvents() throws Exception { .setRunnerName("test") .build(); - service.uploadOutputs(action, spawnResult, () -> {}); + uploadOutputsAndWait(service, action, spawnResult); assertThat(eventHandler.getPosts()) .containsAtLeast( @@ -1937,7 +1937,7 @@ public void uploadOutputs_missingMandatoryOutputs_dontUpload() throws Exception .setRunnerName("test") .build(); - service.uploadOutputs(action, spawnResult, () -> {}); + uploadOutputsAndWait(service, action, spawnResult); // assert assertThat(cache.getNumFindMissingDigests()).isEmpty(); @@ -2578,4 +2578,11 @@ private void createOutputDirectories(Spawn spawn) throws IOException { dir.createDirectoryAndParents(); } } + + private static void uploadOutputsAndWait( + RemoteExecutionService service, RemoteAction action, SpawnResult result) throws Exception { + SettableFuture future = SettableFuture.create(); + service.uploadOutputs(action, result, () -> future.set(null)); + future.get(); + } }