From 2edab739e1f61fe8813230b03396ca46f0790089 Mon Sep 17 00:00:00 2001 From: Ulrik Falklof Date: Mon, 3 Jan 2022 07:58:22 -0800 Subject: [PATCH] Avoid too verbose warnings in terminal when cache issues Deduplicate warnings in terminal. This was working in earlier bazel versions both for read and write, but become broken when write was moved to RemoteExecutionService.java by the "Remote: Async upload" set of commits, completed by commit 581c81a854. Use same phrase "Remote Cache" for both read and write, for deduplication to work better. Avoid printing short warnings on multiple lines for reads, as it already was for writes. Closes #14442. PiperOrigin-RevId: 419442535 --- .../remote/RemoteActionContextProvider.java | 1 - .../lib/remote/RemoteExecutionService.java | 19 ++++++++++-- .../build/lib/remote/RemoteSpawnCache.java | 30 +++---------------- .../lib/remote/RemoteSpawnCacheTest.java | 2 +- .../bazel/remote/remote_execution_test.sh | 4 +-- 5 files changed, 23 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java index 88b2cc951d9b8b..ba37cbba0c390e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java @@ -192,7 +192,6 @@ public void registerSpawnCache(ModuleActionContextRegistry.Builder registryBuild env.getExecRoot(), checkNotNull(env.getOptions().getOptions(RemoteOptions.class)), checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)).verboseFailures, - env.getReporter(), getRemoteExecutionService()); registryBuilder.register(SpawnCache.class, spawnCache, "remote-cache"); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 195e1b6f61663c..0dfcedb90e743d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -132,10 +132,12 @@ import java.util.Collections; import java.util.Comparator; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Objects; +import java.util.Set; import java.util.SortedMap; import java.util.TreeSet; import java.util.concurrent.ConcurrentLinkedQueue; @@ -161,6 +163,7 @@ public class RemoteExecutionService { private final ImmutableSet filesToDownload; @Nullable private final Path captureCorruptedOutputsDir; private final Cache merkleTreeCache; + private final Set reportedErrors = new HashSet<>(); private final Scheduler scheduler; @@ -1186,10 +1189,9 @@ private void reportUploadError(Throwable error) { return; } - String errorMessage = - "Writing to Remote Cache: " + grpcAwareErrorMessage(error, verboseFailures); + String errorMessage = "Remote Cache: " + grpcAwareErrorMessage(error, verboseFailures); - reporter.handle(Event.warn(errorMessage)); + report(Event.warn(errorMessage)); } /** @@ -1312,4 +1314,15 @@ public void shutdown() { remoteExecutor.close(); } } + + void report(Event evt) { + + synchronized (this) { + if (reportedErrors.contains(evt.getMessage())) { + return; + } + reportedErrors.add(evt.getMessage()); + reporter.handle(evt); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index e40969b547c36c..3222d6a1ecdc27 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -31,7 +31,6 @@ import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.events.Event; -import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.SpawnCache; import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent; import com.google.devtools.build.lib.exec.SpawnExecutingEvent; @@ -48,10 +47,7 @@ import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput; import com.google.devtools.build.lib.vfs.Path; import java.io.IOException; -import java.util.HashSet; import java.util.NoSuchElementException; -import java.util.Set; -import javax.annotation.Nullable; /** A remote {@link SpawnCache} implementation. */ @ThreadSafe // If the RemoteActionCache implementation is thread-safe. @@ -66,20 +62,16 @@ final class RemoteSpawnCache implements SpawnCache { private final Path execRoot; private final RemoteOptions options; private final boolean verboseFailures; - @Nullable private final Reporter cmdlineReporter; - private final Set reportedErrors = new HashSet<>(); private final RemoteExecutionService remoteExecutionService; RemoteSpawnCache( Path execRoot, RemoteOptions options, boolean verboseFailures, - @Nullable Reporter cmdlineReporter, RemoteExecutionService remoteExecutionService) { this.execRoot = execRoot; this.options = options; this.verboseFailures = verboseFailures; - this.cmdlineReporter = cmdlineReporter; this.remoteExecutionService = remoteExecutionService; } @@ -150,13 +142,13 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) errorMessage = Utils.grpcAwareErrorMessage(e); } else { // On --verbose_failures print the whole stack trace - errorMessage = Throwables.getStackTraceAsString(e); + errorMessage = "\n" + Throwables.getStackTraceAsString(e); } if (isNullOrEmpty(errorMessage)) { errorMessage = e.getClass().getSimpleName(); } - errorMessage = "Reading from Remote Cache:\n" + errorMessage; - report(Event.warn(errorMessage)); + errorMessage = "Remote Cache: " + errorMessage; + remoteExecutionService.report(Event.warn(errorMessage)); } } } @@ -193,7 +185,7 @@ public void store(SpawnResult result) throws ExecException, InterruptedException try (SilentCloseable c = prof.profile("RemoteCache.checkForConcurrentModifications")) { checkForConcurrentModifications(); } catch (IOException | ForbiddenActionInputException e) { - report(Event.warn(e.getMessage())); + remoteExecutionService.report(Event.warn(e.getMessage())); return; } } @@ -223,20 +215,6 @@ private void checkForConcurrentModifications() } } - private void report(Event evt) { - if (cmdlineReporter == null) { - return; - } - - synchronized (this) { - if (reportedErrors.contains(evt.getMessage())) { - return; - } - reportedErrors.add(evt.getMessage()); - cmdlineReporter.handle(evt); - } - } - @Override public boolean usefulInDynamicExecution() { return false; diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index edf9133ac918e0..4996893f56e73e 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -226,7 +226,7 @@ private RemoteSpawnCache remoteSpawnCacheWithOptions(RemoteOptions options) { null, ImmutableSet.of(), /* captureCorruptedOutputsDir= */ null)); - return new RemoteSpawnCache(execRoot, options, /* verboseFailures=*/ true, reporter, service); + return new RemoteSpawnCache(execRoot, options, /* verboseFailures=*/ true, service); } @Before diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index dfdf71dfe90182..64a9cec6f74365 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3266,14 +3266,14 @@ EOF # Check the error message when failed to upload bazel build --remote_cache=http://nonexistent.example.org //a:foo >& $TEST_log || fail "Failed to build" - expect_log "WARNING: Writing to Remote Cache:" + expect_log "WARNING: Remote Cache:" bazel test \ --remote_cache=grpc://localhost:${worker_port} \ --experimental_remote_cache_async \ --flaky_test_attempts=2 \ //a:test >& $TEST_log && fail "expected failure" || true - expect_not_log "WARNING: Writing to Remote Cache:" + expect_not_log "WARNING: Remote Cache:" } function test_download_toplevel_when_turn_remote_cache_off() {