remote: Replace Retrier with Retrier2.

- Replace the existing Retrier with Retrier2.
- Rename Retrier2 to Retrier and remove the old Retrier + RetryException
class.

RELNOTES: None.
PiperOrigin-RevId: 177835070

6 files changed, 283 insertions, 437 deletions

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 0b70f36623..21e4438d6f 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
@@ -26,6 +26,7 @@ import com.google.common.util.concurrent.ListenableFuture;
 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.vfs.FileSystem.HashFunction;
 import com.google.devtools.remoteexecution.v1test.Digest;
 import com.google.devtools.remoteexecution.v1test.RequestMetadata;
@@ -89,8 +90,7 @@ public class ByteStreamUploaderTest {
   private Channel channel;
   private Context withEmptyMetadata;
 
-  @Mock
-  private Retrier.Backoff mockBackoff;
+  @Mock private Retrier.Backoff mockBackoff;
 
   @Before
   public final void setUp() throws Exception {
@@ -117,7 +117,8 @@ public class ByteStreamUploaderTest {
   @Test(timeout = 10000)
   public void singleBlobUploadShouldWork() throws Exception {
     withEmptyMetadata.attach();
-    Retrier retrier = new Retrier(() -> mockBackoff, (Status s) -> true);
+    RemoteRetrier retrier =
+        new RemoteRetrier(() -> mockBackoff, (e) -> true, Retrier.ALLOW_ALL_CALLS);
     ByteStreamUploader uploader =
         new ByteStreamUploader(INSTANCE_NAME, channel, null, 3, retrier, retryService);
 
@@ -186,7 +187,8 @@ public class ByteStreamUploaderTest {
   @Test(timeout = 20000)
   public void multipleBlobsUploadShouldWork() throws Exception {
     withEmptyMetadata.attach();
-    Retrier retrier = new Retrier(() -> new FixedBackoff(1, 0), (Status s) -> true);
+    RemoteRetrier retrier =
+        new RemoteRetrier(() -> new FixedBackoff(1, 0), (e) -> true, Retrier.ALLOW_ALL_CALLS);
     ByteStreamUploader uploader =
         new ByteStreamUploader(INSTANCE_NAME, channel, null, 3, retrier, retryService);
 
@@ -276,7 +278,8 @@ public class ByteStreamUploaderTest {
     withEmptyMetadata.attach();
     // We upload blobs with different context, and retry 3 times for each upload.
     // We verify that the correct metadata is passed to the server with every blob.
-    Retrier retrier = new Retrier(() -> new FixedBackoff(3, 0), (Status s) -> true);
+    RemoteRetrier retrier =
+        new RemoteRetrier(() -> new FixedBackoff(3, 0), (e) -> true, Retrier.ALLOW_ALL_CALLS);
     ByteStreamUploader uploader =
         new ByteStreamUploader(INSTANCE_NAME, channel, null, 3, retrier, retryService);
 
@@ -365,7 +368,8 @@ public class ByteStreamUploaderTest {
     // Test that uploading the same file concurrently triggers only one file upload.
 
     withEmptyMetadata.attach();
-    Retrier retrier = new Retrier(() -> mockBackoff, (Status s) -> true);
+    RemoteRetrier retrier =
+        new RemoteRetrier(() -> mockBackoff, (e) -> true, Retrier.ALLOW_ALL_CALLS);
     ByteStreamUploader uploader =
         new ByteStreamUploader(INSTANCE_NAME, channel, null, 3, retrier, retryService);
 
@@ -424,7 +428,8 @@ public class ByteStreamUploaderTest {
   @Test(timeout = 10000)
   public void errorsShouldBeReported() throws IOException, InterruptedException {
     withEmptyMetadata.attach();
-    Retrier retrier = new Retrier(() -> new FixedBackoff(1, 10), (Status s) -> true);
+    RemoteRetrier retrier =
+        new RemoteRetrier(() -> new FixedBackoff(1, 10), (e) -> true, Retrier.ALLOW_ALL_CALLS);
     ByteStreamUploader uploader =
         new ByteStreamUploader(INSTANCE_NAME, channel, null, 3, retrier, retryService);
 
@@ -444,14 +449,15 @@ public class ByteStreamUploaderTest {
       fail("Should have thrown an exception.");
     } catch (RetryException e) {
       assertThat(e.getAttempts()).isEqualTo(2);
-      assertThat(e.causedByStatusCode(Code.INTERNAL)).isTrue();
+      assertThat(RemoteRetrierUtils.causedByStatus(e, Code.INTERNAL)).isTrue();
     }
   }
 
   @Test(timeout = 10000)
   public void shutdownShouldCancelOngoingUploads() throws Exception {
     withEmptyMetadata.attach();
-    Retrier retrier = new Retrier(() -> new FixedBackoff(1, 10), (Status s) -> true);
+    RemoteRetrier retrier =
+        new RemoteRetrier(() -> new FixedBackoff(1, 10), (e) -> true, Retrier.ALLOW_ALL_CALLS);
     ByteStreamUploader uploader =
         new ByteStreamUploader(INSTANCE_NAME, channel, null, 3, retrier, retryService);
 
@@ -503,7 +509,8 @@ public class ByteStreamUploaderTest {
   @Test(timeout = 10000)
   public void failureInRetryExecutorShouldBeHandled() throws Exception {
     withEmptyMetadata.attach();
-    Retrier retrier = new Retrier(() -> new FixedBackoff(1, 10), (Status s) -> true);
+    RemoteRetrier retrier =
+        new RemoteRetrier(() -> new FixedBackoff(1, 10), (e) -> true, Retrier.ALLOW_ALL_CALLS);
     ByteStreamUploader uploader =
         new ByteStreamUploader(INSTANCE_NAME, channel, null, 3, retrier, retryService);
 
@@ -534,7 +541,8 @@ public class ByteStreamUploaderTest {
   @Test(timeout = 10000)
   public void resourceNameWithoutInstanceName() throws Exception {
     withEmptyMetadata.attach();
-    Retrier retrier = new Retrier(() -> mockBackoff, (Status s) -> true);
+    RemoteRetrier retrier =
+        new RemoteRetrier(() -> mockBackoff, (e) -> true, Retrier.ALLOW_ALL_CALLS);
     ByteStreamUploader uploader =
         new ByteStreamUploader(/* instanceName */ null, channel, null, 3, retrier, retryService);
 
@@ -571,8 +579,11 @@ public class ByteStreamUploaderTest {
   @Test(timeout = 10000)
   public void nonRetryableStatusShouldNotBeRetried() throws Exception {
     withEmptyMetadata.attach();
-    Retrier retrier = new Retrier(() -> new FixedBackoff(1, 0),
-        /* No Status is retriable. */ (Status s) -> false);
+    RemoteRetrier retrier =
+        new RemoteRetrier(
+            () -> new FixedBackoff(1, 0),
+            /* No Status is retriable. */ (e) -> false,
+            Retrier.ALLOW_ALL_CALLS);
     ByteStreamUploader uploader =
         new ByteStreamUploader(/* instanceName */ null, channel, null, 3, retrier, retryService);
 
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
index 5664ea37cc..d2c8d287c4 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
@@ -152,7 +152,9 @@ public class GrpcRemoteCacheTest {
         scratch.resolve(authTlsOptions.authCredentials).getInputStream(),
         authTlsOptions.authScope);
     RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class);
-    Retrier retrier = new Retrier(remoteOptions);
+    RemoteRetrier retrier =
+        new RemoteRetrier(
+            remoteOptions, RemoteRetrier.RETRIABLE_GRPC_ERRORS, Retrier.ALLOW_ALL_CALLS);
     return new GrpcRemoteCache(
         ClientInterceptors.intercept(
             InProcessChannelBuilder.forName(fakeServerName).directExecutor().build(),
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
index 2e06805539..b6fcd830df 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
@@ -232,7 +232,8 @@ public class GrpcRemoteExecutionClientTest {
     FileSystemUtils.createDirectoryAndParents(stderr.getParentDirectory());
     outErr = new FileOutErr(stdout, stderr);
     RemoteOptions options = Options.getDefaults(RemoteOptions.class);
-    Retrier retrier = new Retrier(options);
+    RemoteRetrier retrier =
+        new RemoteRetrier(options, RemoteRetrier.RETRIABLE_GRPC_ERRORS, Retrier.ALLOW_ALL_CALLS);
     Channel channel = InProcessChannelBuilder.forName(fakeServerName).directExecutor().build();
     GrpcRemoteExecutor executor =
         new GrpcRemoteExecutor(channel, null, options.remoteTimeout, retrier);
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 058a2dfba6..68ce4543cd 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
@@ -20,9 +20,9 @@ import static org.mockito.Mockito.when;
 
 import com.google.common.collect.Range;
 import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff;
-import com.google.devtools.build.lib.remote.Retrier2.Backoff;
-import com.google.devtools.build.lib.remote.Retrier2.RetryException2;
-import com.google.devtools.build.lib.remote.Retrier2.Sleeper;
+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;
@@ -42,7 +42,7 @@ import org.mockito.Mockito;
 public class RemoteRetrierTest {
 
   interface Foo {
-    public String foo();
+    String foo();
   }
 
   private RemoteRetrierTest.Foo fooMock;
@@ -54,7 +54,7 @@ public class RemoteRetrierTest {
 
   @Test
   public void testExponentialBackoff() throws Exception {
-    Retrier2.Backoff backoff =
+    Retrier.Backoff backoff =
         new ExponentialBackoff(Duration.ofSeconds(1), Duration.ofSeconds(10), 2, 0, 6);
     assertThat(backoff.nextDelayMillis()).isEqualTo(1000);
     assertThat(backoff.nextDelayMillis()).isEqualTo(2000);
@@ -67,7 +67,7 @@ public class RemoteRetrierTest {
 
   @Test
   public void testExponentialBackoffJittered() throws Exception {
-    Retrier2.Backoff backoff =
+    Retrier.Backoff backoff =
         new ExponentialBackoff(Duration.ofSeconds(1), Duration.ofSeconds(10), 2, 0.1, 6);
     assertThat(backoff.nextDelayMillis()).isIn(Range.closedOpen(900L, 1100L));
     assertThat(backoff.nextDelayMillis()).isIn(Range.closedOpen(1800L, 2200L));
@@ -82,7 +82,7 @@ public class RemoteRetrierTest {
     try {
       retrier.execute(() -> fooMock.foo());
       fail();
-    } catch (RetryException2 e) {
+    } catch (RetryException e) {
       assertThat(e.getAttempts()).isEqualTo(attempts);
     }
   }
@@ -92,8 +92,8 @@ public class RemoteRetrierTest {
     RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.experimentalRemoteRetry = false;
 
-    RemoteRetrier retrier = Mockito.spy(new RemoteRetrier(options,
-        RemoteRetrier.RETRIABLE_GRPC_ERRORS, Retrier2.ALLOW_ALL_CALLS));
+    RemoteRetrier retrier =
+        Mockito.spy(new RemoteRetrier(options, (e) -> true, Retrier.ALLOW_ALL_CALLS));
     when(fooMock.foo())
         .thenReturn("bla")
         .thenThrow(Status.Code.UNKNOWN.toStatus().asRuntimeException());
@@ -107,7 +107,7 @@ public class RemoteRetrierTest {
     Supplier<Backoff> s =
         () -> new ExponentialBackoff(Duration.ofSeconds(1), Duration.ofSeconds(10), 2.0, 0.0, 2);
     RemoteRetrier retrier = Mockito.spy(new RemoteRetrier(s, (e) -> false,
-        Retrier2.ALLOW_ALL_CALLS, Mockito.mock(Sleeper.class)));
+        Retrier.ALLOW_ALL_CALLS, Mockito.mock(Sleeper.class)));
     when(fooMock.foo()).thenThrow(Status.Code.UNKNOWN.toStatus().asRuntimeException());
     assertThrows(retrier, 1);
     Mockito.verify(fooMock, Mockito.times(1)).foo();
@@ -119,7 +119,7 @@ public class RemoteRetrierTest {
         () -> new ExponentialBackoff(Duration.ofSeconds(1), Duration.ofSeconds(10), 2.0, 0.0, 2);
     Sleeper sleeper = Mockito.mock(Sleeper.class);
     RemoteRetrier retrier = Mockito.spy(new RemoteRetrier(s, (e) -> true,
-        Retrier2.ALLOW_ALL_CALLS, sleeper));
+        Retrier.ALLOW_ALL_CALLS, sleeper));
 
     when(fooMock.foo()).thenThrow(Status.Code.UNKNOWN.toStatus().asRuntimeException());
     assertThrows(retrier, 3);
@@ -135,8 +135,7 @@ public class RemoteRetrierTest {
 
     RemoteOptions options = Options.getDefaults(RemoteOptions.class);
     options.experimentalRemoteRetry = false;
-    RemoteRetrier retrier = new RemoteRetrier(options, RemoteRetrier.RETRIABLE_GRPC_ERRORS,
-        Retrier2.ALLOW_ALL_CALLS);
+    RemoteRetrier retrier = new RemoteRetrier(options, (e) -> true, Retrier.ALLOW_ALL_CALLS);
     try {
       retrier.execute(() -> {
         throw thrown;
@@ -152,8 +151,7 @@ public class RemoteRetrierTest {
     StatusRuntimeException thrown = Status.Code.UNKNOWN.toStatus().asRuntimeException();
 
     RemoteOptions options = Options.getDefaults(RemoteOptions.class);
-    RemoteRetrier retrier = new RemoteRetrier(options, RemoteRetrier.RETRIABLE_GRPC_ERRORS,
-        Retrier2.ALLOW_ALL_CALLS);
+    RemoteRetrier retrier = new RemoteRetrier(options, (e) -> true, Retrier.ALLOW_ALL_CALLS);
 
     AtomicInteger numCalls = new AtomicInteger();
     try {
@@ -162,7 +160,7 @@ public class RemoteRetrierTest {
         throw new RemoteRetrier.PassThroughException(thrown);
       });
       fail();
-    } catch (RetryException2 expected) {
+    } catch (RetryException expected) {
       assertThat(expected).hasCauseThat().isSameAs(thrown);
     }
 
diff --git a/src/test/java/com/google/devtools/build/lib/remote/Retrier2Test.java b/src/test/java/com/google/devtools/build/lib/remote/Retrier2Test.java
deleted file mode 100644
index 220c825cda..0000000000
--- a/src/test/java/com/google/devtools/build/lib/remote/Retrier2Test.java
+++ /dev/null
@@ -1,307 +0,0 @@
-// Copyright 2016 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.devtools.build.lib.remote;
-
-import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.fail;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
-
-import com.google.devtools.build.lib.remote.Retrier2.Backoff;
-import com.google.devtools.build.lib.remote.Retrier2.CircuitBreaker;
-import com.google.devtools.build.lib.remote.Retrier2.CircuitBreaker.State;
-import com.google.devtools.build.lib.remote.Retrier2.CircuitBreakerException;
-import com.google.devtools.build.lib.remote.Retrier2.RetryException2;
-import java.util.concurrent.atomic.AtomicInteger;
-import java.util.function.Predicate;
-import java.util.function.Supplier;
-import javax.annotation.concurrent.ThreadSafe;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-import org.mockito.Mock;
-import org.mockito.MockitoAnnotations;
-
-/**
- * Tests for {@link Retrier2}.
- */
-@RunWith(JUnit4.class)
-public class Retrier2Test {
-
-  @Mock
-  private CircuitBreaker alwaysOpen;
-
-  private static final Predicate<Exception> RETRY_ALL = (e) -> true;
-  private static final Predicate<Exception> RETRY_NONE = (e) -> false;
-
-  @Before
-  public void setup() {
-    MockitoAnnotations.initMocks(this);
-    when(alwaysOpen.state()).thenReturn(State.ACCEPT_CALLS);
-  }
-
-  @Test
-  public void retryShouldWork_failure() throws Exception {
-    // Test that a call is retried according to the backoff.
-    // All calls fail.
-
-    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/2);
-    Retrier2 r = new Retrier2(s, RETRY_ALL, alwaysOpen);
-    try {
-      r.execute(() -> {
-        throw new Exception("call failed");
-      });
-      fail("exception expected.");
-    } catch (RetryException2 e) {
-      assertThat(e.getAttempts()).isEqualTo(3);
-    }
-
-    verify(alwaysOpen, times(3)).recordFailure();
-    verify(alwaysOpen, never()).recordSuccess();
-  }
-
-  @Test
-  public void retryShouldWorkNoRetries_failure() throws Exception {
-    // Test that a non-retriable error is not retried.
-    // All calls fail.
-
-    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/2);
-    Retrier2 r = new Retrier2(s, RETRY_NONE, alwaysOpen);
-    try {
-      r.execute(() -> {
-        throw new Exception("call failed");
-      });
-      fail("exception expected.");
-    } catch (RetryException2 e) {
-      assertThat(e.getAttempts()).isEqualTo(1);
-    }
-
-    verify(alwaysOpen, times(1)).recordFailure();
-    verify(alwaysOpen, never()).recordSuccess();
-  }
-
-  @Test
-  public void retryShouldWork_success() throws Exception {
-    // Test that a call is retried according to the backoff.
-    // The last call succeeds.
-
-    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/2);
-    Retrier2 r = new Retrier2(s, RETRY_ALL, alwaysOpen);
-    AtomicInteger numCalls = new AtomicInteger();
-    int val = r.execute(() -> {
-      numCalls.incrementAndGet();
-      if (numCalls.get() == 3) {
-        return 1;
-      }
-      throw new Exception("call failed");
-    });
-    assertThat(val).isEqualTo(1);
-
-    verify(alwaysOpen, times(2)).recordFailure();
-    verify(alwaysOpen, times(1)).recordSuccess();
-  }
-
-  @Test
-  public void nestedRetriesShouldWork() throws Exception {
-    // Test that nested calls using retries compose as expected.
-
-    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/1);
-    Retrier2 r = new Retrier2(s, RETRY_ALL, alwaysOpen);
-
-    AtomicInteger attemptsLvl0 = new AtomicInteger();
-    AtomicInteger attemptsLvl1 = new AtomicInteger();
-    AtomicInteger attemptsLvl2 = new AtomicInteger();
-    try {
-      r.execute(() -> {
-        attemptsLvl0.incrementAndGet();
-        return r.execute(() -> {
-          attemptsLvl1.incrementAndGet();
-          return r.execute(() -> {
-            attemptsLvl2.incrementAndGet();
-            throw new Exception("call failed");
-          });
-        });
-      });
-    } catch (RetryException2 outer) {
-      assertThat(outer.getAttempts()).isEqualTo(2);
-      assertThat(outer).hasCauseThat().hasMessageThat().isEqualTo("call failed");
-      assertThat(attemptsLvl0.get()).isEqualTo(2);
-      assertThat(attemptsLvl1.get()).isEqualTo(4);
-      assertThat(attemptsLvl2.get()).isEqualTo(8);
-    }
-  }
-
-  @Test
-  public void circuitBreakerShouldTrip() throws Exception {
-    // Test that a circuit breaker can trip.
-
-    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/3);
-    TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2);
-    Retrier2 r = new Retrier2(s, RETRY_ALL, cb);
-
-    try {
-      r.execute(() -> {
-        throw new Exception("call failed");
-      });
-      fail ("exception expected");
-    } catch (CircuitBreakerException expected) {
-      // Intentionally left empty.
-    }
-
-    assertThat(cb.state()).isEqualTo(State.REJECT_CALLS);
-    assertThat(cb.consecutiveFailures).isEqualTo(2);
-  }
-
-  @Test
-  public void circuitBreakerCanRecover() throws Exception {
-    // Test that a circuit breaker can recover from REJECT_CALLS to ACCEPT_CALLS by
-    // utilizing the TRIAL_CALL state.
-
-    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/3);
-    TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2);
-    Retrier2 r = new Retrier2(s, RETRY_ALL, cb);
-
-    cb.trialCall();
-
-    assertThat(cb.state()).isEqualTo(State.TRIAL_CALL);
-
-    int val = r.execute(() -> 10);
-    assertThat(val).isEqualTo(10);
-    assertThat(cb.state()).isEqualTo(State.ACCEPT_CALLS);
-  }
-
-  @Test
-  public void circuitBreakerHalfOpenIsNotRetried() throws Exception {
-    // Test that a call executed in TRIAL_CALL state is not retried
-    // in case of failure.
-
-    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/3);
-    TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2);
-    Retrier2 r = new Retrier2(s, RETRY_ALL, cb);
-
-    cb.trialCall();
-
-    try {
-      r.execute(() -> {
-        throw new Exception("call failed");
-      });
-    } catch (RetryException2 expected) {
-      // Intentionally left empty.
-    }
-
-    assertThat(cb.consecutiveFailures).isEqualTo(1);
-  }
-
-  @Test
-  public void interruptsShouldNotBeRetried_flag() throws Exception {
-    // Test that a call is not executed / retried if the current thread
-    // is interrupted.
-
-    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/3);
-    TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2);
-    Retrier2 r = new Retrier2(s, RETRY_ALL, cb);
-
-    try {
-      Thread.currentThread().interrupt();
-      r.execute(() -> 10);
-    } catch (InterruptedException expected) {
-      // Intentionally left empty.
-    }
-  }
-
-  @Test
-  public void interruptsShouldNotBeRetried_exception() throws Exception {
-    // Test that a call is not retried if an InterruptedException is thrown.
-
-    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/3);
-    TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2);
-    Retrier2 r = new Retrier2(s, RETRY_ALL, cb);
-
-    try {
-      Thread.currentThread().interrupt();
-      r.execute(() -> {
-        throw new InterruptedException();
-      });
-    } catch (InterruptedException expected) {
-      // Intentionally left empty.
-    }
-  }
-
-  /**
-   * Simple circuit breaker that trips after N consecutive failures.
-   */
-  @ThreadSafe
-  private static class TripAfterNCircuitBreaker implements CircuitBreaker {
-
-    private final int maxConsecutiveFailures;
-
-    private State state = State.ACCEPT_CALLS;
-    private int consecutiveFailures;
-
-    TripAfterNCircuitBreaker(int maxConsecutiveFailures) {
-      this.maxConsecutiveFailures = maxConsecutiveFailures;
-    }
-
-    @Override
-    public synchronized State state() {
-      return state;
-    }
-
-    @Override
-    public synchronized void recordFailure() {
-      consecutiveFailures++;
-      if (consecutiveFailures >= maxConsecutiveFailures) {
-        state = State.REJECT_CALLS;
-      }
-    }
-
-    @Override
-    public synchronized void recordSuccess() {
-      consecutiveFailures = 0;
-      state = State.ACCEPT_CALLS;
-    }
-
-    void trialCall() {
-      state = State.TRIAL_CALL;
-    }
-  }
-
-  private static class ZeroBackoff implements Backoff {
-
-    private final int maxRetries;
-    private int retries;
-
-    public ZeroBackoff(int maxRetries) {
-      this.maxRetries = maxRetries;
-    }
-
-    @Override
-    public long nextDelayMillis() {
-      if (retries >= maxRetries) {
-        return -1;
-      }
-      retries++;
-      return 0;
-    }
-
-    @Override
-    public int getRetryAttempts() {
-      return retries;
-    }
-  }
-}
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 dfce640c06..945c27d66d 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
@@ -1,4 +1,4 @@
-// Copyright 2015 The Bazel Authors. All rights reserved.
+// Copyright 2016 The Bazel Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -11,156 +11,297 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
+
 package com.google.devtools.build.lib.remote;
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.fail;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
-import com.google.common.collect.Range;
-import io.grpc.Status;
-import io.grpc.StatusRuntimeException;
-import java.io.IOException;
-import java.time.Duration;
+import com.google.devtools.build.lib.remote.Retrier.Backoff;
+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 java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+import javax.annotation.concurrent.ThreadSafe;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
-import org.mockito.Mockito;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
 
-/** Tests for {@link Retrier}. */
+/**
+ * Tests for {@link Retrier}.
+ */
 @RunWith(JUnit4.class)
 public class RetrierTest {
 
-  interface Foo {
-    public String foo();
-  }
+  @Mock
+  private CircuitBreaker alwaysOpen;
 
-  private Foo fooMock;
+  private static final Predicate<Exception> RETRY_ALL = (e) -> true;
+  private static final Predicate<Exception> RETRY_NONE = (e) -> false;
 
   @Before
-  public void setUp() {
-    fooMock = Mockito.mock(Foo.class);
+  public void setup() {
+    MockitoAnnotations.initMocks(this);
+    when(alwaysOpen.state()).thenReturn(State.ACCEPT_CALLS);
   }
 
   @Test
-  public void testExponentialBackoff() throws Exception {
-    Retrier.Backoff backoff =
-        Retrier.Backoff.exponential(
-                Duration.ofSeconds(1), Duration.ofSeconds(10), 2, 0, 6)
-            .get();
-    assertThat(backoff.nextDelayMillis()).isEqualTo(1000);
-    assertThat(backoff.nextDelayMillis()).isEqualTo(2000);
-    assertThat(backoff.nextDelayMillis()).isEqualTo(4000);
-    assertThat(backoff.nextDelayMillis()).isEqualTo(8000);
-    assertThat(backoff.nextDelayMillis()).isEqualTo(10000);
-    assertThat(backoff.nextDelayMillis()).isEqualTo(10000);
-    assertThat(backoff.nextDelayMillis()).isEqualTo(Retrier.Backoff.STOP);
+  public void retryShouldWork_failure() throws Exception {
+    // Test that a call is retried according to the backoff.
+    // All calls fail.
+
+    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/2);
+    Retrier r = new Retrier(s, RETRY_ALL, alwaysOpen);
+    try {
+      r.execute(() -> {
+        throw new Exception("call failed");
+      });
+      fail("exception expected.");
+    } catch (RetryException e) {
+      assertThat(e.getAttempts()).isEqualTo(3);
+    }
+
+    verify(alwaysOpen, times(3)).recordFailure();
+    verify(alwaysOpen, never()).recordSuccess();
   }
 
   @Test
-  public void testExponentialBackoffJittered() throws Exception {
-    Retrier.Backoff backoff =
-        Retrier.Backoff.exponential(
-                Duration.ofSeconds(1), Duration.ofSeconds(10), 2, 0.1, 6)
-            .get();
-    assertThat(backoff.nextDelayMillis()).isIn(Range.closedOpen(900L, 1100L));
-    assertThat(backoff.nextDelayMillis()).isIn(Range.closedOpen(1800L, 2200L));
-    assertThat(backoff.nextDelayMillis()).isIn(Range.closedOpen(3600L, 4400L));
-    assertThat(backoff.nextDelayMillis()).isIn(Range.closedOpen(7200L, 8800L));
-    assertThat(backoff.nextDelayMillis()).isIn(Range.closedOpen(9000L, 11000L));
-    assertThat(backoff.nextDelayMillis()).isIn(Range.closedOpen(9000L, 11000L));
-    assertThat(backoff.nextDelayMillis()).isEqualTo(Retrier.Backoff.STOP);
-  }
+  public void retryShouldWorkNoRetries_failure() throws Exception {
+    // Test that a non-retriable error is not retried.
+    // All calls fail.
 
-  void assertThrows(Retrier retrier, int attempts) throws InterruptedException, IOException {
+    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/2);
+    Retrier r = new Retrier(s, RETRY_NONE, alwaysOpen);
     try {
-      retrier.execute(() -> fooMock.foo());
-      fail();
+      r.execute(() -> {
+        throw new Exception("call failed");
+      });
+      fail("exception expected.");
     } catch (RetryException e) {
-      assertThat(e.getAttempts()).isEqualTo(attempts);
+      assertThat(e.getAttempts()).isEqualTo(1);
     }
+
+    verify(alwaysOpen, times(1)).recordFailure();
+    verify(alwaysOpen, never()).recordSuccess();
   }
 
   @Test
-  public void testNoRetries() throws Exception {
-    Retrier retrier = Mockito.spy(Retrier.NO_RETRIES);
-    Mockito.doNothing().when(retrier).sleep(Mockito.anyLong());
-    when(fooMock.foo())
-        .thenReturn("bla")
-        .thenThrow(Status.Code.UNKNOWN.toStatus().asRuntimeException());
-    assertThat(retrier.execute(() -> fooMock.foo())).isEqualTo("bla");
-    assertThrows(retrier, 1);
-    Mockito.verify(fooMock, Mockito.times(2)).foo();
+  public void retryShouldWork_success() throws Exception {
+    // Test that a call is retried according to the backoff.
+    // The last call succeeds.
+
+    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/2);
+    Retrier r = new Retrier(s, RETRY_ALL, alwaysOpen);
+    AtomicInteger numCalls = new AtomicInteger();
+    int val = r.execute(() -> {
+      numCalls.incrementAndGet();
+      if (numCalls.get() == 3) {
+        return 1;
+      }
+      throw new Exception("call failed");
+    });
+    assertThat(val).isEqualTo(1);
+
+    verify(alwaysOpen, times(2)).recordFailure();
+    verify(alwaysOpen, times(1)).recordSuccess();
   }
 
   @Test
-  public void testNonRetriableError() throws Exception {
-    Retrier retrier =
-        Mockito.spy(
-            new Retrier(
-                Retrier.Backoff.exponential(
-                    Duration.ofSeconds(1), Duration.ofSeconds(10), 2, 0, 2),
-                Retrier.DEFAULT_IS_RETRIABLE));
-    Mockito.doNothing().when(retrier).sleep(Mockito.anyLong());
-    when(fooMock.foo()).thenThrow(Status.Code.NOT_FOUND.toStatus().asRuntimeException());
-    assertThrows(retrier, 1);
-    Mockito.verify(fooMock, Mockito.times(1)).foo();
+  public void nestedRetriesShouldWork() throws Exception {
+    // Test that nested calls using retries compose as expected.
+
+    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/1);
+    Retrier r = new Retrier(s, RETRY_ALL, alwaysOpen);
+
+    AtomicInteger attemptsLvl0 = new AtomicInteger();
+    AtomicInteger attemptsLvl1 = new AtomicInteger();
+    AtomicInteger attemptsLvl2 = new AtomicInteger();
+    try {
+      r.execute(() -> {
+        attemptsLvl0.incrementAndGet();
+        return r.execute(() -> {
+          attemptsLvl1.incrementAndGet();
+          return r.execute(() -> {
+            attemptsLvl2.incrementAndGet();
+            throw new Exception("call failed");
+          });
+        });
+      });
+    } catch (RetryException outer) {
+      assertThat(outer.getAttempts()).isEqualTo(2);
+      assertThat(outer).hasCauseThat().hasMessageThat().isEqualTo("call failed");
+      assertThat(attemptsLvl0.get()).isEqualTo(2);
+      assertThat(attemptsLvl1.get()).isEqualTo(4);
+      assertThat(attemptsLvl2.get()).isEqualTo(8);
+    }
+  }
+
+  @Test
+  public void circuitBreakerShouldTrip() throws Exception {
+    // Test that a circuit breaker can trip.
+
+    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/3);
+    TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2);
+    Retrier r = new Retrier(s, RETRY_ALL, cb);
+
+    try {
+      r.execute(() -> {
+        throw new Exception("call failed");
+      });
+      fail ("exception expected");
+    } catch (CircuitBreakerException expected) {
+      // Intentionally left empty.
+    }
+
+    assertThat(cb.state()).isEqualTo(State.REJECT_CALLS);
+    assertThat(cb.consecutiveFailures).isEqualTo(2);
   }
 
   @Test
-  public void testRepeatedRetriesReset() throws Exception {
-    Retrier retrier =
-        Mockito.spy(
-            new Retrier(
-                Retrier.Backoff.exponential(
-                    Duration.ofSeconds(1), Duration.ofSeconds(10), 2, 0, 2),
-                Retrier.RETRY_ALL));
-    Mockito.doNothing().when(retrier).sleep(Mockito.anyLong());
-    when(fooMock.foo()).thenThrow(Status.Code.UNKNOWN.toStatus().asRuntimeException());
-    assertThrows(retrier, 3);
-    assertThrows(retrier, 3);
-    Mockito.verify(retrier, Mockito.times(2)).sleep(1000);
-    Mockito.verify(retrier, Mockito.times(2)).sleep(2000);
-    Mockito.verify(fooMock, Mockito.times(6)).foo();
+  public void circuitBreakerCanRecover() throws Exception {
+    // Test that a circuit breaker can recover from REJECT_CALLS to ACCEPT_CALLS by
+    // utilizing the TRIAL_CALL state.
+
+    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/3);
+    TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2);
+    Retrier r = new Retrier(s, RETRY_ALL, cb);
+
+    cb.trialCall();
+
+    assertThat(cb.state()).isEqualTo(State.TRIAL_CALL);
+
+    int val = r.execute(() -> 10);
+    assertThat(val).isEqualTo(10);
+    assertThat(cb.state()).isEqualTo(State.ACCEPT_CALLS);
   }
 
   @Test
-  public void testInterruptedExceptionIsPassedThrough() throws Exception {
-    InterruptedException thrown = new InterruptedException();
+  public void circuitBreakerHalfOpenIsNotRetried() throws Exception {
+    // Test that a call executed in TRIAL_CALL state is not retried
+    // in case of failure.
+
+    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/3);
+    TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2);
+    Retrier r = new Retrier(s, RETRY_ALL, cb);
+
+    cb.trialCall();
+
     try {
-      Retrier.NO_RETRIES.execute(() -> {
-        throw thrown;
+      r.execute(() -> {
+        throw new Exception("call failed");
       });
-      fail();
-    } catch (InterruptedException expected) {
-      assertThat(expected).isSameAs(thrown);
+    } catch (RetryException expected) {
+      // Intentionally left empty.
     }
+
+    assertThat(cb.consecutiveFailures).isEqualTo(1);
   }
 
   @Test
-  public void testPassThroughException() throws Exception {
-    StatusRuntimeException thrown = Status.Code.UNKNOWN.toStatus().asRuntimeException();
+  public void interruptsShouldNotBeRetried_flag() throws Exception {
+    // Test that a call is not executed / retried if the current thread
+    // is interrupted.
+
+    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/3);
+    TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2);
+    Retrier r = new Retrier(s, RETRY_ALL, cb);
+
     try {
-      Retrier.NO_RETRIES.execute(() -> {
-        throw new Retrier.PassThroughException(thrown);
-      });
-      fail();
-    } catch (StatusRuntimeException expected) {
-      assertThat(expected).isSameAs(thrown);
+      Thread.currentThread().interrupt();
+      r.execute(() -> 10);
+    } catch (InterruptedException expected) {
+      // Intentionally left empty.
     }
   }
 
   @Test
-  public void testIOExceptionIsPassedThrough() throws Exception {
-    IOException thrown = new IOException();
+  public void interruptsShouldNotBeRetried_exception() throws Exception {
+    // Test that a call is not retried if an InterruptedException is thrown.
+
+    Supplier<Backoff> s  = () -> new ZeroBackoff(/*maxRetries=*/3);
+    TripAfterNCircuitBreaker cb = new TripAfterNCircuitBreaker(/*maxConsecutiveFailures=*/2);
+    Retrier r = new Retrier(s, RETRY_ALL, cb);
+
     try {
-      Retrier.NO_RETRIES.execute(() -> {
-        throw thrown;
+      Thread.currentThread().interrupt();
+      r.execute(() -> {
+        throw new InterruptedException();
       });
-      fail();
-    } catch (IOException expected) {
-      assertThat(expected).isSameAs(thrown);
+    } catch (InterruptedException expected) {
+      // Intentionally left empty.
+    }
+  }
+
+  /**
+   * Simple circuit breaker that trips after N consecutive failures.
+   */
+  @ThreadSafe
+  private static class TripAfterNCircuitBreaker implements CircuitBreaker {
+
+    private final int maxConsecutiveFailures;
+
+    private State state = State.ACCEPT_CALLS;
+    private int consecutiveFailures;
+
+    TripAfterNCircuitBreaker(int maxConsecutiveFailures) {
+      this.maxConsecutiveFailures = maxConsecutiveFailures;
+    }
+
+    @Override
+    public synchronized State state() {
+      return state;
+    }
+
+    @Override
+    public synchronized void recordFailure() {
+      consecutiveFailures++;
+      if (consecutiveFailures >= maxConsecutiveFailures) {
+        state = State.REJECT_CALLS;
+      }
+    }
+
+    @Override
+    public synchronized void recordSuccess() {
+      consecutiveFailures = 0;
+      state = State.ACCEPT_CALLS;
+    }
+
+    void trialCall() {
+      state = State.TRIAL_CALL;
+    }
+  }
+
+  private static class ZeroBackoff implements Backoff {
+
+    private final int maxRetries;
+    private int retries;
+
+    public ZeroBackoff(int maxRetries) {
+      this.maxRetries = maxRetries;
+    }
+
+    @Override
+    public long nextDelayMillis() {
+      if (retries >= maxRetries) {
+        return -1;
+      }
+      retries++;
+      return 0;
+    }
+
+    @Override
+    public int getRetryAttempts() {
+      return retries;
     }
   }
 }