Skip to content

Commit

Permalink
Avoid too verbose warnings in terminal when cache issues (#14504)
Browse files Browse the repository at this point in the history
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

Co-authored-by: Ulrik Falklof <[email protected]>
  • Loading branch information
coeuvre and ulrfa authored Jan 4, 2022
1 parent dc59d9e commit d7f1341
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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));
}

/**
Expand Down Expand Up @@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand All @@ -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;
}

Expand Down Expand Up @@ -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));
}
}
}
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down

0 comments on commit d7f1341

Please sign in to comment.