Avoid too verbose warnings in terminal when cache issues

ulrfa · copybara-github · commit 2edab739e1f6 · 2022-01-03T07:59:33.000-08:00
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 581c81a. 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
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
@@ -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
@@ -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<PathFragment> filesToDownload;
   @Nullable private final Path captureCorruptedOutputsDir;
   private final Cache<Object, MerkleTree> merkleTreeCache;
+  private final Set<String> 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
@@ -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<String> 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
@@ -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
@@ -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() {

Original file line number	Diff line number	Diff line change
`@@ -192,7 +192,6 @@ public void registerSpawnCache(ModuleActionContextRegistry.Builder registryBuild`
`192`	`192`	`env.getExecRoot(),`
`193`	`193`	`checkNotNull(env.getOptions().getOptions(RemoteOptions.class)),`
`194`	`194`	`checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)).verboseFailures,`
`195`		`- env.getReporter(),`
`196`	`195`	`getRemoteExecutionService());`
`197`	`196`	`registryBuilder.register(SpawnCache.class, spawnCache, "remote-cache");`
`198`	`197`	`}`
Original file line number	Diff line number	Diff line change
`@@ -226,7 +226,7 @@ private RemoteSpawnCache remoteSpawnCacheWithOptions(RemoteOptions options) {`
`226`	`226`	`null,`
`227`	`227`	`ImmutableSet.of(),`
`228`	`228`	`/* captureCorruptedOutputsDir= */ null));`
`229`		`- return new RemoteSpawnCache(execRoot, options, /* verboseFailures=*/ true, reporter, service);`
	`229`	`+ return new RemoteSpawnCache(execRoot, options, /* verboseFailures=*/ true, service);`
`230`	`230`	`}`
`231`	`231`
`232`	`232`	`@Before`