From 1532df0e5d1978e885879f872e4128c1deb59964 Mon Sep 17 00:00:00 2001 From: Benjamin Peterson Date: Thu, 24 Jan 2019 08:45:44 -0800 Subject: [PATCH] Remove RetryException. Retries should be an implementation detail of the cache client implementation. No client is interested in the number of retry attempts, so it seems better to make the retry code transparent with respect to exceptions. The retry code also wrapped arbitrary exceptions with RetryException. It is now the responsibility of the cache implementation code to convert low-level exceptions to IOException or an error meaningful at higher layers like CacheNotFoundException. For example, the gRPC action cache and remote executor now explicitly convert StatusRuntimeException into IOException. Also, remove PassThroughException, since it's unused. This should fix https://github.com/bazelbuild/bazel/issues/6308. Closes #7009. PiperOrigin-RevId: 230728718 --- .../lib/remote/AbstractRemoteActionCache.java | 4 - .../build/lib/remote/ByteStreamUploader.java | 19 +-- .../build/lib/remote/GrpcRemoteCache.java | 29 +++-- .../build/lib/remote/GrpcRemoteExecutor.java | 84 +++++++------ .../build/lib/remote/RemoteRetrier.java | 56 +++------ .../build/lib/remote/RemoteRetrierUtils.java | 16 ++- .../lib/remote/RemoteServerCapabilities.java | 3 + .../build/lib/remote/RemoteSpawnCache.java | 10 +- .../build/lib/remote/RemoteSpawnRunner.java | 21 +--- .../devtools/build/lib/remote/Retrier.java | 97 +++------------ ...eStreamBuildEventArtifactUploaderTest.java | 2 - .../lib/remote/ByteStreamUploaderTest.java | 10 +- .../build/lib/remote/RemoteRetrierTest.java | 42 +------ .../lib/remote/RemoteSpawnCacheTest.java | 7 +- .../lib/remote/RemoteSpawnRunnerTest.java | 23 +--- .../build/lib/remote/RetrierTest.java | 117 +++++++++--------- 16 files changed, 203 insertions(+), 337 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java index 93c1463a53bde1..fff89e0a7f198c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java @@ -71,10 +71,6 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { ((SettableFuture) EMPTY_BYTES).set(new byte[0]); } - public static boolean causedByCacheMiss(IOException t) { - return t.getCause() instanceof CacheNotFoundException; - } - protected final RemoteOptions options; protected final DigestUtil digestUtil; private final Retrier retrier; diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamUploader.java index 278d19d0f456fa..e459bba6bd8c43 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamUploader.java @@ -33,7 +33,6 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.SettableFuture; -import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import io.grpc.CallCredentials; import io.grpc.CallOptions; @@ -42,6 +41,7 @@ import io.grpc.Context; import io.grpc.Metadata; import io.grpc.Status; +import io.grpc.StatusRuntimeException; import io.netty.util.AbstractReferenceCounted; import io.netty.util.ReferenceCounted; import java.io.IOException; @@ -130,7 +130,6 @@ public ByteStreamUploader( * @param forceUpload if {@code false} the blob is not uploaded if it has previously been * uploaded, if {@code true} the blob is uploaded. * @throws IOException when reading of the {@link Chunker}s input source fails - * @throws RetryException when the upload failed after a retry */ public void uploadBlob(Chunker chunker, boolean forceUpload) throws IOException, InterruptedException { @@ -151,12 +150,11 @@ public void uploadBlob(Chunker chunker, boolean forceUpload) throws IOException, * * @param chunkers the data to upload. * @param forceUpload if {@code false} the blob is not uploaded if it has previously been - * uploaded, if {@code true} the blob is uploaded. - * @throws IOException when reading of the {@link Chunker}s input source fails - * @throws RetryException when the upload failed after a retry + * uploaded, if {@code true} the blob is uploaded. + * @throws IOException when reading of the {@link Chunker}s input source or uploading fails */ - public void uploadBlobs(Iterable chunkers, boolean forceUpload) throws IOException, - InterruptedException { + public void uploadBlobs(Iterable chunkers, boolean forceUpload) + throws IOException, InterruptedException { List> uploads = new ArrayList<>(); for (Chunker chunker : chunkers) { @@ -170,7 +168,11 @@ public void uploadBlobs(Iterable chunkers, boolean forceUpload) throws } catch (ExecutionException e) { Throwable cause = e.getCause(); Throwables.propagateIfInstanceOf(cause, IOException.class); - throw new RuntimeException(cause); + Throwables.propagateIfInstanceOf(cause, InterruptedException.class); + if (cause instanceof StatusRuntimeException) { + throw new IOException(cause); + } + Throwables.propagate(cause); } } @@ -212,7 +214,6 @@ void shutdown() { * @param forceUpload if {@code false} the blob is not uploaded if it has previously been * uploaded, if {@code true} the blob is uploaded. * @throws IOException when reading of the {@link Chunker}s input source fails - * @throws RetryException when the upload failed after a retry */ public ListenableFuture uploadBlobAsync(Chunker chunker, boolean forceUpload) { Digest digest = checkNotNull(chunker.digest()); diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java index 75b528adb503a7..537050980cc7a7 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java @@ -44,7 +44,6 @@ import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; -import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey; @@ -323,15 +322,19 @@ public void upload( if (!uploadAction) { return; } - retrier.execute( - () -> - acBlockingStub() - .updateActionResult( - UpdateActionResultRequest.newBuilder() - .setInstanceName(options.remoteInstanceName) - .setActionDigest(actionKey.getDigest()) - .setActionResult(result) - .build())); + try { + retrier.execute( + () -> + acBlockingStub() + .updateActionResult( + UpdateActionResultRequest.newBuilder() + .setInstanceName(options.remoteInstanceName) + .setActionDigest(actionKey.getDigest()) + .setActionResult(result) + .build())); + } catch (StatusRuntimeException e) { + throw new IOException(e); + } } void upload( @@ -433,12 +436,12 @@ public ActionResult getCachedActionResult(ActionKey actionKey) .setInstanceName(options.remoteInstanceName) .setActionDigest(actionKey.getDigest()) .build())); - } catch (RetryException e) { - if (RemoteRetrierUtils.causedByStatus(e, Status.Code.NOT_FOUND)) { + } catch (StatusRuntimeException e) { + if (e.getStatus().getCode() == Status.Code.NOT_FOUND) { // Return null to indicate that it was a cache miss. return null; } - throw e; + throw new IOException(e); } } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java index 799356f4b3966a..02ea99b510ce88 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java @@ -136,52 +136,56 @@ public ExecuteResponse executeRemotely(ExecuteRequest request) new AtomicReference<>(Operation.getDefaultInstance()); final AtomicBoolean waitExecution = new AtomicBoolean(false); // Whether we should call WaitExecution. - return retrier.execute( - () -> { - final Iterator replies; - if (waitExecution.get()) { - WaitExecutionRequest wr = - WaitExecutionRequest.newBuilder().setName(operation.get().getName()).build(); - replies = execBlockingStub().waitExecution(wr); - } else { - replies = execBlockingStub().execute(request); - } - try { - while (replies.hasNext()) { - Operation o = replies.next(); - operation.set(o); - waitExecution.set(!operation.get().getDone()); - ExecuteResponse r = getOperationResponse(o); - if (r != null) { - return r; - } - } - } catch (StatusRuntimeException e) { - if (e.getStatus().getCode() == Code.NOT_FOUND) { - // Operation was lost on the server. Retry Execute. - waitExecution.set(false); + try { + return retrier.execute( + () -> { + final Iterator replies; + if (waitExecution.get()) { + WaitExecutionRequest wr = + WaitExecutionRequest.newBuilder().setName(operation.get().getName()).build(); + replies = execBlockingStub().waitExecution(wr); + } else { + replies = execBlockingStub().execute(request); } - throw e; - } finally { - // The blocking streaming call closes correctly only when trailers and a Status - // are received from the server so that onClose() is called on this call's - // CallListener. Under normal circumstances (no cancel/errors), these are - // guaranteed to be sent by the server only if replies.hasNext() has been called - // after all replies from the stream have been consumed. try { while (replies.hasNext()) { - replies.next(); + Operation o = replies.next(); + operation.set(o); + waitExecution.set(!operation.get().getDone()); + ExecuteResponse r = getOperationResponse(o); + if (r != null) { + return r; + } } } catch (StatusRuntimeException e) { - // Cleanup: ignore exceptions, because the meaningful errors have already been - // propagated. + if (e.getStatus().getCode() == Code.NOT_FOUND) { + // Operation was lost on the server. Retry Execute. + waitExecution.set(false); + } + throw e; + } finally { + // The blocking streaming call closes correctly only when trailers and a Status + // are received from the server so that onClose() is called on this call's + // CallListener. Under normal circumstances (no cancel/errors), these are + // guaranteed to be sent by the server only if replies.hasNext() has been called + // after all replies from the stream have been consumed. + try { + while (replies.hasNext()) { + replies.next(); + } + } catch (StatusRuntimeException e) { + // Cleanup: ignore exceptions, because the meaningful errors have already been + // propagated. + } } - } - throw new IOException( - String.format( - "Remote server error: execution request for %s terminated with no result.", - operation.get().getName())); - }); + throw new IOException( + String.format( + "Remote server error: execution request for %s terminated with no result.", + operation.get().getName())); + }); + } catch (StatusRuntimeException e) { + throw new IOException(e); + } } public void close() { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrier.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrier.java index c0d4906678dda6..104f660694a789 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrier.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrier.java @@ -16,43 +16,21 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.base.Throwables; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import io.grpc.Status; import io.grpc.StatusException; import io.grpc.StatusRuntimeException; +import java.io.IOException; import java.time.Duration; import java.util.concurrent.Callable; import java.util.concurrent.ThreadLocalRandom; import java.util.function.Predicate; import java.util.function.Supplier; -/** - * Specific retry logic for remote execution/caching. - * - *

A call can disable retries by throwing a {@link PassThroughException}. - * RemoteRetrier r = ...; - * try { - * r.execute(() -> { - * // Not retried. - * throw PassThroughException(new IOException("fail")); - * } - * } catch (RetryException e) { - * // e.getCause() is the IOException - * System.out.println(e.getCause()); - * } - * - */ +/** Specific retry logic for remote execution/caching. */ public class RemoteRetrier extends Retrier { - /** - * Wraps around an {@link Exception} to make it pass through a single layer of retries. - */ - public static class PassThroughException extends Exception { - public PassThroughException(Exception e) { - super(e); - } - } - public static final Predicate RETRIABLE_GRPC_ERRORS = e -> { if (!(e instanceof StatusException) && !(e instanceof StatusRuntimeException)) { @@ -102,7 +80,7 @@ public RemoteRetrier( Predicate shouldRetry, ListeningScheduledExecutorService retryScheduler, CircuitBreaker circuitBreaker) { - super(backoff, supportPassthrough(shouldRetry), retryScheduler, circuitBreaker); + super(backoff, shouldRetry, retryScheduler, circuitBreaker); } @VisibleForTesting @@ -112,30 +90,24 @@ public RemoteRetrier( ListeningScheduledExecutorService retryScheduler, CircuitBreaker circuitBreaker, Sleeper sleeper) { - super(backoff, supportPassthrough(shouldRetry), retryScheduler, circuitBreaker, sleeper); + super(backoff, shouldRetry, retryScheduler, circuitBreaker, sleeper); } + /** + * Execute a callable with retries. {@link IOException} and {@link InterruptedException} are + * propagated directly to the caller. All other exceptions are wrapped in {@link RuntimeError}. + */ @Override - public T execute(Callable call) throws RetryException, InterruptedException { + public T execute(Callable call) throws IOException, InterruptedException { try { return super.execute(call); - } catch (RetryException e) { - if (e.getCause() instanceof PassThroughException) { - PassThroughException passThrough = (PassThroughException) e.getCause(); - throw new RetryException("Retries aborted because of PassThroughException", - e.getAttempts(), (Exception) passThrough.getCause()); - } - throw e; + } catch (Exception e) { + Throwables.propagateIfInstanceOf(e, IOException.class); + Throwables.propagateIfInstanceOf(e, InterruptedException.class); + throw Throwables.propagate(e); } } - - private static Predicate supportPassthrough( - Predicate delegate) { - // PassThroughException is not retriable. - return e -> !(e instanceof PassThroughException) && delegate.test(e); - } - static class ExponentialBackoff implements Retrier.Backoff { private final long maxMillis; diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrierUtils.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrierUtils.java index 720ff4c7bb0946..dac7bf76c64936 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrierUtils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrierUtils.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.remote; -import com.google.devtools.build.lib.remote.Retrier.RetryException; import io.grpc.Status; import io.grpc.StatusException; import io.grpc.StatusRuntimeException; @@ -23,23 +22,22 @@ public final class RemoteRetrierUtils { public static boolean causedByStatus(Throwable e, Status.Code expected) { - if (e instanceof RetryException) { - e = e.getCause(); - } if (e instanceof StatusRuntimeException) { return ((StatusRuntimeException) e).getStatus().getCode() == expected; } else if (e instanceof StatusException) { return ((StatusException) e).getStatus().getCode() == expected; + } else if (e.getCause() != null) { + return causedByStatus(e.getCause(), expected); } return false; } public static boolean causedByExecTimeout(Throwable e) { - if (!(e instanceof RetryException)) { - return false; + if (e instanceof ExecutionStatusException) { + return ((ExecutionStatusException) e).isExecutionTimeout(); + } else if (e.getCause() != null) { + return causedByExecTimeout(e.getCause()); } - e = e.getCause(); - return (e instanceof ExecutionStatusException - && ((ExecutionStatusException) e).isExecutionTimeout()); + return false; } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java index a8f36ddddd16e1..40a68762ae3489 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import io.grpc.CallCredentials; import io.grpc.Context; +import io.grpc.StatusRuntimeException; import java.io.IOException; import java.util.List; import java.util.concurrent.TimeUnit; @@ -73,6 +74,8 @@ public ServerCapabilities get(String buildRequestId, String commandId) ? GetCapabilitiesRequest.getDefaultInstance() : GetCapabilitiesRequest.newBuilder().setInstanceName(instanceName).build(); return retrier.execute(() -> capabilitiesBlockingStub().getCapabilities(request)); + } catch (StatusRuntimeException e) { + throw new IOException(e); } finally { withMetadata.detach(previous); } 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 d941685b99ff1f..66a0b23ab41fc7 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 @@ -158,15 +158,15 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) .build(); return SpawnCache.success(spawnResult); } + } catch (CacheNotFoundException e) { + // Intentionally left blank } catch (IOException e) { - if (!AbstractRemoteActionCache.causedByCacheMiss(e)) { - String errorMsg = e.getMessage(); - if (isNullOrEmpty(errorMsg)) { + String errorMsg = e.getMessage(); + if (isNullOrEmpty(errorMsg)) { errorMsg = e.getClass().getSimpleName(); - } + } errorMsg = "Reading from Remote Cache:\n" + errorMsg; report(Event.warn(errorMsg)); - } } finally { withMetadata.detach(previous); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 792c7aa365fc7a..55d0cdd3a4635a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -52,7 +52,6 @@ import com.google.devtools.build.lib.exec.SpawnRunner; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; -import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey; @@ -204,10 +203,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) .setCacheHit(true) .setRunnerName("remote cache hit") .build(); - } catch (RetryException e) { - if (!AbstractRemoteActionCache.causedByCacheMiss(e)) { - throw e; - } + } catch (CacheNotFoundException e) { // No cache hit, so we fall through to local or remote execution. // We set acceptCachedResult to false in order to force the action re-execution. acceptCachedResult = false; @@ -357,9 +353,7 @@ private SpawnResult execLocallyAndUploadOrFail( if (Thread.currentThread().isInterrupted()) { throw new InterruptedException(); } - if (remoteOptions.remoteLocalFallback - && !(cause instanceof RetryException - && RemoteRetrierUtils.causedByExecTimeout((RetryException) cause))) { + if (remoteOptions.remoteLocalFallback && !RemoteRetrierUtils.causedByExecTimeout(cause)) { return execLocallyAndUpload( spawn, context, inputMap, remoteCache, actionKey, action, command, uploadLocalResults); } @@ -368,9 +362,8 @@ private SpawnResult execLocallyAndUploadOrFail( private SpawnResult handleError(IOException exception, FileOutErr outErr, ActionKey actionKey) throws ExecException, InterruptedException, IOException { - final Throwable cause = exception.getCause(); - if (cause instanceof ExecutionStatusException) { - ExecutionStatusException e = (ExecutionStatusException) cause; + if (exception.getCause() instanceof ExecutionStatusException) { + ExecutionStatusException e = (ExecutionStatusException) exception.getCause(); if (e.getResponse() != null) { ExecuteResponse resp = e.getResponse(); maybeDownloadServerLogs(resp, actionKey); @@ -388,11 +381,9 @@ private SpawnResult handleError(IOException exception, FileOutErr outErr, Action } } final Status status; - if (exception instanceof RetryException - && RemoteRetrierUtils.causedByStatus((RetryException) exception, Code.UNAVAILABLE)) { + if (RemoteRetrierUtils.causedByStatus(exception, Code.UNAVAILABLE)) { status = Status.EXECUTION_FAILED_CATASTROPHICALLY; - } else if (exception instanceof CacheNotFoundException - || cause instanceof CacheNotFoundException) { + } else if (exception instanceof CacheNotFoundException) { status = Status.REMOTE_CACHE_FAILED; } else { status = Status.EXECUTION_FAILED; diff --git a/src/main/java/com/google/devtools/build/lib/remote/Retrier.java b/src/main/java/com/google/devtools/build/lib/remote/Retrier.java index 8863438d3e6c22..05dee217a56b6c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/Retrier.java +++ b/src/main/java/com/google/devtools/build/lib/remote/Retrier.java @@ -16,6 +16,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.base.Throwables; import com.google.common.util.concurrent.AsyncCallable; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; @@ -65,7 +66,7 @@ public interface Backoff { *

The initial state of a circuit breaker is the {@link State#ACCEPT_CALLS}. Calls are executed * and retried in this state. However, if error rates are high a circuit breaker can choose to * transition into {@link State#REJECT_CALLS}. In this state any calls are rejected with a {@link - * RetryException} immediately. A circuit breaker in state {@link State#REJECT_CALLS} can + * CircuitBreakerException} immediately. A circuit breaker in state {@link State#REJECT_CALLS} can * periodically return a {@code TRIAL_CALL} state, in which case a call will be executed once and * in case of success the circuit breaker may return to state {@code ACCEPT_CALLS}. * @@ -109,6 +110,13 @@ enum State { void recordSuccess(); } + /** Thrown if the call was stopped by a circuit breaker. */ + public static class CircuitBreakerException extends IOException { + private CircuitBreakerException() { + super("Call not executed due to a high failure rate."); + } + } + /** * {@link Sleeper#sleep(long)} is called to pause between synchronous retries ({@link * #execute(Callable)}. @@ -117,45 +125,6 @@ public interface Sleeper { void sleep(long millis) throws InterruptedException; } - /** - * Wraps around the actual cause for the retry. Contains information about the number of retry - * attempts. - */ - public static class RetryException extends IOException { - - private final int attempts; - - public RetryException(String message, int numRetries, Exception cause) { - super(message, cause); - this.attempts = numRetries + 1; - } - - protected RetryException(String message) { - super(message); - this.attempts = 0; - } - - /** - * Returns the number of times a {@link Callable} has been executed before this exception was - * thrown. - */ - public int getAttempts() { - return attempts; - } - } - - /** Thrown if the call was stopped by a circuit breaker. */ - public static class CircuitBreakerException extends RetryException { - - private CircuitBreakerException(String message, int numRetries, Exception cause) { - super(message, numRetries, cause); - } - - private CircuitBreakerException() { - super("Call not executed due to a high failure rate."); - } - } - /** Disables circuit breaking. */ public static final CircuitBreaker ALLOW_ALL_CALLS = new CircuitBreaker() { @@ -246,14 +215,14 @@ public Retrier( *

{@link InterruptedException} is not retried. * * @param call the {@link Callable} to execute. - * @throws RetryException if the {@code call} didn't succeed within the framework specified by - * {@code backoffSupplier} and {@code shouldRetry}. + * @throws Exception if the {@code call} didn't succeed within the framework specified by {@code + * backoffSupplier} and {@code shouldRetry}. * @throws CircuitBreakerException in case a call was rejected because the circuit breaker * tripped. * @throws InterruptedException if the {@code call} throws an {@link InterruptedException} or the * current thread's interrupted flag is set. */ - public T execute(Callable call) throws RetryException, InterruptedException { + public T execute(Callable call) throws Exception { final Backoff backoff = newBackoff(); while (true) { final State circuitState; @@ -268,31 +237,18 @@ public T execute(Callable call) throws RetryException, InterruptedExcepti T r = call.call(); circuitBreaker.recordSuccess(); return r; - } catch (InterruptedException e) { - circuitBreaker.recordFailure(); - throw e; } catch (Exception e) { circuitBreaker.recordFailure(); - Exception orig = e; - if (e instanceof RetryException) { - // Support nested retry calls. - e = (Exception) e.getCause(); - } + Throwables.propagateIfInstanceOf(e, InterruptedException.class); if (State.TRIAL_CALL.equals(circuitState)) { - throw new CircuitBreakerException( - "Call failed in circuit breaker half open state.", 0, e); + throw e; } - int attempts = backoff.getRetryAttempts(); if (!shouldRetry.test(e)) { - throw new RetryException( - "Call failed with not retriable error: " + orig.getMessage(), attempts, e); + throw e; } final long delayMillis = backoff.nextDelayMillis(); if (delayMillis < 0) { - throw new RetryException( - "Call failed after " + attempts + " retry attempts: " + orig.getMessage(), - attempts, - e); + throw e; } sleeper.sleep(delayMillis); } @@ -364,31 +320,16 @@ public void onSuccess(Object o) { @Override public void onFailure(Throwable t) { - Exception e = t instanceof Exception ? (Exception) t : new Exception(t); - outerF.setException( - new RetryException( - "Scheduled execution errored.", backoff.getRetryAttempts(), e)); + outerF.setException(t); } }, MoreExecutors.directExecutor()); } catch (RejectedExecutionException e) { // May be thrown by .schedule(...) if i.e. the executor is shutdown. - outerF.setException( - new RetryException("Rejected by executor.", backoff.getRetryAttempts(), e)); + outerF.setException(new IOException(e)); } } else { - Exception e = t instanceof Exception ? (Exception) t : new Exception(t); - String message = - waitMillis >= 0 - ? "Status not retriable" - : "Exhausted retry attempts (" + backoff.getRetryAttempts() + ")"; - if (!e.getMessage().isEmpty()) { - message += ": " + e.getMessage(); - } else { - message += "."; - } - RetryException error = new RetryException(message, backoff.getRetryAttempts(), e); - outerF.setException(error); + outerF.setException(t); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index 08e1c9593bab87..87c35dde98de29 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -29,7 +29,6 @@ import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.remote.ByteStreamUploaderTest.FixedBackoff; import com.google.devtools.build.lib.remote.ByteStreamUploaderTest.MaybeFailOnceUploadService; -import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -240,7 +239,6 @@ public void onCompleted() { artifactUploader.upload(filesToUpload).get(); fail("exception expected."); } catch (ExecutionException e) { - assertThat(e.getCause()).isInstanceOf(RetryException.class); assertThat(Status.fromThrowable(e).getCode()).isEqualTo(Status.CANCELLED.getCode()); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java index b2e5f565d75101..76a426ec6d7c1a 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java @@ -28,7 +28,6 @@ import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.analysis.BlazeVersionInfo; -import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -418,8 +417,7 @@ public StreamObserver write(StreamObserver response try { uploader.uploadBlob(chunker, true); fail("Should have thrown an exception."); - } catch (RetryException e) { - assertThat(e.getAttempts()).isEqualTo(2); + } catch (IOException e) { assertThat(RemoteRetrierUtils.causedByStatus(e, Code.INTERNAL)).isTrue(); } @@ -517,7 +515,7 @@ public StreamObserver write(StreamObserver response try { uploader.uploadBlob(chunker, true); fail("Should have thrown an exception."); - } catch (RetryException e) { + } catch (IOException e) { assertThat(e).hasCauseThat().isInstanceOf(RejectedExecutionException.class); } @@ -595,7 +593,7 @@ public StreamObserver write(StreamObserver response try { uploader.uploadBlob(chunker, true); fail("Should have thrown an exception."); - } catch (RetryException e) { + } catch (IOException e) { assertThat(numCalls.get()).isEqualTo(1); } @@ -662,7 +660,7 @@ public void onCompleted() { try { // This should fail uploader.uploadBlob(chunker, true); - } catch (RetryException e) { + } catch (IOException e) { if (e.getCause() instanceof StatusRuntimeException) { expected = (StatusRuntimeException) e.getCause(); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteRetrierTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteRetrierTest.java index d9b08d26ce5627..7a5d67c5a55eeb 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteRetrierTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteRetrierTest.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows; import static org.junit.Assert.fail; import static org.mockito.Mockito.when; @@ -23,14 +24,12 @@ import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff; import com.google.devtools.build.lib.remote.Retrier.Backoff; -import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.Retrier.Sleeper; import com.google.devtools.common.options.Options; import io.grpc.Status; import io.grpc.StatusRuntimeException; import java.time.Duration; import java.util.concurrent.Executors; -import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; import org.junit.AfterClass; import org.junit.Before; @@ -94,15 +93,6 @@ public void testExponentialBackoffJittered() throws Exception { assertThat(backoff.nextDelayMillis()).isLessThan(0L); } - private void assertThrows(RemoteRetrier retrier, int attempts) throws Exception { - try { - retrier.execute(() -> fooMock.foo()); - fail(); - } catch (RetryException e) { - assertThat(e.getAttempts()).isEqualTo(attempts); - } - } - @Test public void testNoRetries() throws Exception { RemoteOptions options = Options.getDefaults(RemoteOptions.class); @@ -114,7 +104,7 @@ public void testNoRetries() throws Exception { .thenReturn("bla") .thenThrow(Status.Code.UNKNOWN.toStatus().asRuntimeException()); assertThat(retrier.execute(() -> fooMock.foo())).isEqualTo("bla"); - assertThrows(retrier, 1); + assertThrows(StatusRuntimeException.class, () -> retrier.execute(fooMock::foo)); Mockito.verify(fooMock, Mockito.times(2)).foo(); } @@ -131,7 +121,7 @@ public void testNonRetriableError() throws Exception { Retrier.ALLOW_ALL_CALLS, Mockito.mock(Sleeper.class))); when(fooMock.foo()).thenThrow(Status.Code.UNKNOWN.toStatus().asRuntimeException()); - assertThrows(retrier, 1); + assertThrows(StatusRuntimeException.class, () -> retrier.execute(fooMock::foo)); Mockito.verify(fooMock, Mockito.times(1)).foo(); } @@ -145,8 +135,8 @@ public void testRepeatedRetriesReset() throws Exception { new RemoteRetrier(s, (e) -> true, retryService, Retrier.ALLOW_ALL_CALLS, sleeper)); when(fooMock.foo()).thenThrow(Status.Code.UNKNOWN.toStatus().asRuntimeException()); - assertThrows(retrier, 3); - assertThrows(retrier, 3); + assertThrows(StatusRuntimeException.class, () -> retrier.execute(fooMock::foo)); + assertThrows(StatusRuntimeException.class, () -> retrier.execute(fooMock::foo)); Mockito.verify(sleeper, Mockito.times(2)).sleep(1000); Mockito.verify(sleeper, Mockito.times(2)).sleep(2000); Mockito.verify(fooMock, Mockito.times(6)).foo(); @@ -169,26 +159,4 @@ public void testInterruptedExceptionIsPassedThrough() throws Exception { assertThat(expected).isSameAs(thrown); } } - - @Test - public void testPassThroughException() throws Exception { - StatusRuntimeException thrown = Status.Code.UNKNOWN.toStatus().asRuntimeException(); - - RemoteOptions options = Options.getDefaults(RemoteOptions.class); - RemoteRetrier retrier = - new RemoteRetrier(options, (e) -> true, retryService, Retrier.ALLOW_ALL_CALLS); - - AtomicInteger numCalls = new AtomicInteger(); - try { - retrier.execute(() -> { - numCalls.incrementAndGet(); - throw new RemoteRetrier.PassThroughException(thrown); - }); - fail(); - } catch (RetryException expected) { - assertThat(expected).hasCauseThat().isSameAs(thrown); - } - - assertThat(numCalls.get()).isEqualTo(1); - } } 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 30fbac1c493c38..34b9d8d739547e 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 @@ -52,7 +52,6 @@ import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.exec.util.FakeOwner; -import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; @@ -396,7 +395,7 @@ public void printWarningIfUploadFails() throws Exception { @Test public void printWarningIfDownloadFails() throws Exception { - doThrow(new RetryException("reason", 0, io.grpc.Status.UNAVAILABLE.asRuntimeException())) + doThrow(new IOException(io.grpc.Status.UNAVAILABLE.asRuntimeException())) .when(remoteCache) .getCachedActionResult(any(ActionKey.class)); @@ -443,7 +442,7 @@ public Void answer(InvocationOnMock invocation) { assertThat(eventHandler.getEvents()).hasSize(1); Event evt = eventHandler.getEvents().get(0); assertThat(evt.getKind()).isEqualTo(EventKind.WARNING); - assertThat(evt.getMessage()).contains("reason"); + assertThat(evt.getMessage()).contains("UNAVAILABLE"); assertThat(progressUpdates) .containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache")); } @@ -466,7 +465,7 @@ public ActionResult answer(InvocationOnMock invocation) { return actionResult; } }); - doThrow(new RetryException("cache miss", 0, new CacheNotFoundException(digest, digestUtil))) + doThrow(new CacheNotFoundException(digest, digestUtil)) .when(remoteCache) .download(actionResult, execRoot, outErr); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index 8f391d3258b6e6..93df866fd00026 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows; import static java.nio.charset.StandardCharsets.ISO_8859_1; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; @@ -359,12 +360,7 @@ public void dontAcceptFailedCachedAction() throws Exception { digestUtil, logDir)); - try { - runner.exec(spawn, policy); - fail("Expected exception"); - } catch (EnvironmentalExecException expected) { - // Intentionally left empty. - } + assertThrows(EnvironmentalExecException.class, () -> runner.exec(spawn, policy)); } @Test @@ -624,8 +620,7 @@ public void testHumanReadableServerLogsSavedForFailingActionWithStatus() throws .setStatus(timeoutStatus) .build(); when(executor.executeRemotely(any(ExecuteRequest.class))) - .thenThrow(new Retrier.RetryException( - "", 1, new ExecutionStatusException(resp.getStatus(), resp))); + .thenThrow(new IOException(new ExecutionStatusException(resp.getStatus(), resp))); SettableFuture completed = SettableFuture.create(); completed.set(null); when(cache.downloadFile(eq(logPath), eq(logDigest))).thenReturn(completed); @@ -742,9 +737,7 @@ public void cacheDownloadFailureTriggersRemoteExecution() throws Exception { ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); when(cache.getCachedActionResult(any(ActionKey.class))).thenReturn(cachedResult); - Retrier.RetryException downloadFailure = - new Retrier.RetryException( - "", 1, new CacheNotFoundException(Digest.getDefaultInstance(), digestUtil)); + Exception downloadFailure = new CacheNotFoundException(Digest.getDefaultInstance(), digestUtil); doThrow(downloadFailure) .when(cache) .download(eq(cachedResult), any(Path.class), any(FileOutErr.class)); @@ -797,9 +790,7 @@ public void testRemoteExecutionTimeout() throws Exception { .build()) .build(); when(executor.executeRemotely(any(ExecuteRequest.class))) - .thenThrow( - new Retrier.RetryException( - "", 1, new ExecutionStatusException(resp.getStatus(), resp))); + .thenThrow(new IOException(new ExecutionStatusException(resp.getStatus(), resp))); Spawn spawn = newSimpleSpawn(); @@ -846,9 +837,7 @@ public void testRemoteExecutionTimeoutDoesNotTriggerFallback() throws Exception .build()) .build(); when(executor.executeRemotely(any(ExecuteRequest.class))) - .thenThrow( - new Retrier.RetryException( - "", 1, new ExecutionStatusException(resp.getStatus(), resp))); + .thenThrow(new IOException(new ExecutionStatusException(resp.getStatus(), resp))); Spawn spawn = newSimpleSpawn(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java b/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java index ba88e123949932..4bab6dff19ddcd 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows; import static org.junit.Assert.fail; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -28,7 +29,6 @@ import com.google.devtools.build.lib.remote.Retrier.CircuitBreaker; import com.google.devtools.build.lib.remote.Retrier.CircuitBreaker.State; import com.google.devtools.build.lib.remote.Retrier.CircuitBreakerException; -import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.Retrier.ZeroBackoff; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; @@ -82,15 +82,19 @@ public void retryShouldWork_failure() throws Exception { Supplier s = () -> new ZeroBackoff(/*maxRetries=*/2); Retrier r = new Retrier(s, RETRY_ALL, retryService, alwaysOpen); + AtomicInteger numCalls = new AtomicInteger(); try { - r.execute(() -> { - throw new Exception("call failed"); - }); + r.execute( + () -> { + numCalls.incrementAndGet(); + throw new Exception("call failed"); + }); fail("exception expected."); - } catch (RetryException e) { - assertThat(e.getAttempts()).isEqualTo(3); + } catch (Exception e) { + assertThat(e).hasMessageThat().isEqualTo("call failed"); } + assertThat(numCalls.get()).isEqualTo(3); verify(alwaysOpen, times(3)).recordFailure(); verify(alwaysOpen, never()).recordSuccess(); } @@ -102,15 +106,19 @@ public void retryShouldWorkNoRetries_failure() throws Exception { Supplier s = () -> new ZeroBackoff(/*maxRetries=*/2); Retrier r = new Retrier(s, RETRY_NONE, retryService, alwaysOpen); + AtomicInteger numCalls = new AtomicInteger(); try { - r.execute(() -> { - throw new Exception("call failed"); - }); + r.execute( + () -> { + numCalls.incrementAndGet(); + throw new Exception("call failed"); + }); fail("exception expected."); - } catch (RetryException e) { - assertThat(e.getAttempts()).isEqualTo(1); + } catch (Exception e) { + assertThat(e).hasMessageThat().isEqualTo("call failed"); } + assertThat(numCalls.get()).isEqualTo(1); verify(alwaysOpen, times(1)).recordFailure(); verify(alwaysOpen, never()).recordSuccess(); } @@ -160,17 +168,8 @@ public void nestedRetriesShouldWork() throws Exception { }); }); }); - } catch (RetryException outer) { - assertThat(outer.getAttempts()).isEqualTo(2); - // Propagate original cause. - assertThat(outer).hasCauseThat().hasMessageThat().isEqualTo("failure message"); - // Compose the overall error message. - assertThat(outer) - .hasMessageThat() - .isEqualTo( - "Call failed after 1 retry attempts: " - + "Call failed after 1 retry attempts: " - + "Call failed after 1 retry attempts: failure message"); + } catch (Exception e) { + assertThat(e).hasMessageThat().isEqualTo("failure message"); assertThat(attemptsLvl0.get()).isEqualTo(2); assertThat(attemptsLvl1.get()).isEqualTo(4); assertThat(attemptsLvl2.get()).isEqualTo(8); @@ -228,10 +227,11 @@ public void circuitBreakerHalfOpenIsNotRetried() throws Exception { cb.trialCall(); try { - r.execute(() -> { - throw new Exception("call failed"); - }); - } catch (RetryException expected) { + r.execute( + () -> { + throw new Exception("call failed"); + }); + } catch (Exception expected) { // Intentionally left empty. } @@ -243,34 +243,41 @@ public void interruptsShouldNotBeRetried_flag() throws Exception { // Test that a call is not executed / retried if the current thread // is interrupted. - Supplier s = () -> new ZeroBackoff(/*maxRetries=*/3); - TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2); + Supplier s = () -> new ZeroBackoff(/*maxRetries=*/ 3); + TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/ 2); Retrier r = new Retrier(s, RETRY_ALL, retryService, cb); - try { - Thread.currentThread().interrupt(); - r.execute(() -> 10); - } catch (InterruptedException expected) { - // Intentionally left empty. - } + AtomicInteger numCalls = new AtomicInteger(); + Thread.currentThread().interrupt(); + assertThrows( + InterruptedException.class, + () -> + r.execute( + () -> { + numCalls.incrementAndGet(); + return 10; + })); + assertThat(numCalls.get()).isEqualTo(0); } @Test public void interruptsShouldNotBeRetried_exception() throws Exception { // Test that a call is not retried if an InterruptedException is thrown. - Supplier s = () -> new ZeroBackoff(/*maxRetries=*/3); - TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2); + Supplier s = () -> new ZeroBackoff(/*maxRetries=*/ 3); + TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/ 2); Retrier r = new Retrier(s, RETRY_ALL, retryService, cb); - try { - Thread.currentThread().interrupt(); - r.execute(() -> { - throw new InterruptedException(); - }); - } catch (InterruptedException expected) { - // Intentionally left empty. - } + AtomicInteger numCalls = new AtomicInteger(); + assertThrows( + InterruptedException.class, + () -> + r.execute( + () -> { + numCalls.incrementAndGet(); + throw new InterruptedException(); + })); + assertThat(numCalls.get()).isEqualTo(1); } @Test @@ -280,19 +287,19 @@ public void asyncRetryExhaustRetries() throws Exception { Supplier s = () -> new ZeroBackoff(/*maxRetries=*/ 2); Retrier r = new Retrier(s, RETRY_ALL, retryService, alwaysOpen); + AtomicInteger numCalls = new AtomicInteger(); ListenableFuture res = r.executeAsync( () -> { + numCalls.incrementAndGet(); throw new Exception("call failed"); }); try { res.get(); fail("exception expected."); } catch (ExecutionException e) { - assertThat(e).hasCauseThat().isInstanceOf(RetryException.class); - assertThat(((RetryException) e.getCause()).getAttempts()).isEqualTo(3); - assertThat(e).hasCauseThat().hasMessageThat().contains("Exhausted retry attempts"); - assertThat(e).hasCauseThat().hasMessageThat().contains("call failed"); + assertThat(numCalls.get()).isEqualTo(3); + assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("call failed"); } } @@ -303,18 +310,19 @@ public void asyncRetryNonRetriable() throws Exception { Supplier s = () -> new ZeroBackoff(/*maxRetries=*/ 2); Retrier r = new Retrier(s, RETRY_NONE, retryService, alwaysOpen); + AtomicInteger numCalls = new AtomicInteger(); ListenableFuture res = r.executeAsync( () -> { + numCalls.incrementAndGet(); throw new Exception("call failed"); }); try { res.get(); fail("exception expected."); } catch (ExecutionException e) { - assertThat(e).hasCauseThat().isInstanceOf(RetryException.class); - assertThat(e).hasCauseThat().hasMessageThat().contains("not retriable"); - assertThat(e).hasCauseThat().hasMessageThat().contains("call failed"); + assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("call failed"); + assertThat(numCalls.get()).isEqualTo(1); } } @@ -334,14 +342,11 @@ public void asyncRetryEmptyError() throws Exception { res.get(); fail("exception expected."); } catch (ExecutionException e) { - assertThat(e).hasCauseThat().isInstanceOf(RetryException.class); - assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("Status not retriable."); + assertThat(e).hasCauseThat().hasMessageThat().isEqualTo(""); } } - /** - * Simple circuit breaker that trips after N consecutive failures. - */ + /** Simple circuit breaker that trips after N consecutive failures. */ @ThreadSafe private static class TripAfterNCircuitBreaker implements CircuitBreaker {