From d7f134110631641ea8c3f9b19b37165bb177ef2e Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 4 Jan 2022 13:20:46 +0100 Subject: [PATCH] Avoid too verbose warnings in terminal when cache issues (#14504) 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 Co-authored-by: Ulrik Falklof --- .../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 8cb9a9e8762716..b16be3bbd4c0cf 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; @@ -1184,10 +1187,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)); } /** @@ -1310,4 +1312,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 4171d01fe5b763..da10d379004642 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 344a85a047bca1..03068d27ec06ee 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3230,14 +3230,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() {