Reconcile RetrySettings & Clock #235

vam-google · 2017-03-11T01:10:44Z

Reconcile RetrySettings in GAX with RetryParams in google-cloud-java
googleapis/google-cloud-java#1574
Reconcile NanoClock in GAX with Clock in google-cloud-java
googleapis/google-cloud-java#1575

Reconcile RetrySettings in GAX with RetryParams in google-cloud-java googleapis/google-cloud-java#1574 Reconcile NanoClock in GAX with Clock in google-cloud-java googleapis/google-cloud-java#1575

codecov-io · 2017-03-11T01:23:09Z

Codecov Report

Merging #235 into master will increase coverage by 0.76%.
The diff coverage is 88.42%.

@@             Coverage Diff              @@
##             master     #235      +/-   ##
============================================
+ Coverage     70.27%   71.03%   +0.76%     
- Complexity      478      523      +45     
============================================
  Files            68       75       +7     
  Lines          2476     2593     +117     
  Branches        253      267      +14     
============================================
+ Hits           1740     1842     +102     
- Misses          639      648       +9     
- Partials         97      103       +6

Impacted Files	Coverage Δ	Complexity Δ
...gax/core/internal/ApiFutureToListenableFuture.java	`60% <ø> (ø)`	`3 <0> (ø)`	⬇️
...le/api/gax/retrying/ScheduledRetryingExecutor.java	`100% <100%> (ø)`	`3 <3> (?)`
.../google/api/gax/retrying/TimedAttemptSettings.java	`100% <100%> (ø)`	`7 <7> (?)`
...le/api/gax/retrying/ExponentialRetryAlgorithm.java	`100% <100%> (ø)`	`8 <8> (?)`
...in/java/com/google/api/gax/grpc/UnaryCallable.java	`92% <100%> (+2%)`	`17 <1> (ø)`	⬇️
...va/com/google/api/gax/retrying/RetryAlgorithm.java	`100% <100%> (ø)`	`7 <7> (?)`
...c/main/java/com/google/api/gax/core/NanoClock.java	`66.66% <60%> (ø)`	`4 <3> (?)`
...in/java/com/google/api/gax/core/RetrySettings.java	`41.02% <60%> (-0.65%)`	`3 <2> (ø)`
...oogle/api/gax/retrying/DirectRetryingExecutor.java	`81.25% <81.25%> (ø)`	`4 <4> (?)`
...om/google/api/gax/retrying/RetryingFutureImpl.java	`82.25% <82.25%> (ø)`	`13 <13> (?)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cbfcc3...db34591. Read the comment docs.

garrettjonesgoogle

I started this review on Friday, so apologies if I call out any issues that have already been resolved with PR updates.

src/main/java/com/google/api/gax/core/DefaultNanoClock.java

@@ -0,0 +1,61 @@
+/*
+ * Copyright 2016, Google Inc. All rights reserved.


src/main/java/com/google/api/gax/core/SystemClock.java

+import java.io.Serializable;
+import java.util.concurrent.TimeUnit;
+
+public final class SystemClock implements NanoClock, Serializable {


src/main/java/com/google/api/gax/retrying/AbstractRetryHandler.java

+ *
+ * @param <ResponseT> response type
+ */
+public abstract class AbstractRetryHandler<ResponseT> implements RetryHandler<ResponseT> {


src/main/java/com/google/api/gax/retrying/AbstractRetryHandler.java

+  }
+
+  /**
+   * Ensures that the retry logic hasn't exceeded neither maximum number of retries nor the total


src/main/java/com/google/api/gax/retrying/RetryAttemptSettings.java

+/**
+ * Execution attempt settings. Defines attempt-specific properties of a retry process.
+ */
+public class RetryAttemptSettings {


src/test/java/com/google/api/gax/grpc/RecordingScheduler.java

-    clock.setCurrentNanoTime(clock.nanoTime() + TimeUnit.NANOSECONDS.convert(delay, unit));
-    return executor.schedule(runnable, 0, TimeUnit.NANOSECONDS);
-  }
+  static RecordingScheduler get(final FakeNanoClock clock) {


src/test/java/com/google/api/gax/grpc/CancellationTest.java

-    @Override
-    public List<Runnable> shutdownNow() {
-      return executor.shutdownNow();
+  private abstract static class LatchCountDownScheduler implements ScheduledExecutorService {


src/main/java/com/google/api/gax/core/RetrySettings.java

@@ -92,6 +95,13 @@
  public abstract Duration getMaxRetryDelay();

  /**
+   * MaxAttempts defines the maximum number of attempts to perform. If number of attempts reaches


src/main/java/com/google/api/gax/core/RetrySettings.java

@@ -92,6 +95,13 @@
  public abstract Duration getMaxRetryDelay();

  /**
+   * MaxAttempts defines the maximum number of attempts to perform. If number of attempts reaches
+   * this limit the logic will give up retrying even if the total retry time is still lower than


src/main/java/com/google/api/gax/grpc/RetryingCallable.java

-  private static boolean canRetry(Throwable throwable) {
-    if (!(throwable instanceof ApiException)) {
-      return false;
+  private static class GrpcRetryCallable<RequestT, ResponseT> implements Callable<ResponseT> {


src/main/java/com/google/api/gax/core/RetrySettings.java

@@ -92,6 +95,13 @@
  public abstract Duration getMaxRetryDelay();

  /**
+   * MaxAttempts defines the maximum number of attempts to perform. If number of attempts reaches
+   * this limit the logic will give up retrying even if the total retry time is still lower than
+   * TotalTimeout.


src/main/java/com/google/api/gax/grpc/RetryingCallable.java

-    return resultFuture;
+    GrpcRetryCallable<RequestT, ResponseT> retryCallable =
+        new GrpcRetryCallable<>(callable, request, context);
+    GrpcRetryHandler<ResponseT> retryHelper = new GrpcRetryHandler<>(clock, scheduler);


src/main/java/com/google/api/gax/grpc/RetryingCallable.java

+    GrpcRetryCallable<RequestT, ResponseT> retryCallable =
+        new GrpcRetryCallable<>(callable, request, context);
+    GrpcRetryHandler<ResponseT> retryHelper = new GrpcRetryHandler<>(clock, scheduler);
+    RetryFuture<ResponseT> rv = retryHelper.createFirstAttempt(retryCallable, retryParams);


src/main/java/com/google/api/gax/grpc/RetryingCallable.java

+    @Override
+    public RetryAttemptSettings createNextAttemptSettings(
+        Throwable e, RetryAttemptSettings prevSettings) {
+      if (((ApiException) e).getStatusCode() == Code.DEADLINE_EXCEEDED) {


src/main/java/com/google/api/gax/retrying/AbstractRetryHandler.java

+ *
+ * @param <ResponseT> response type
+ */
+public abstract class AbstractRetryHandler<ResponseT> implements RetryHandler<ResponseT> {


vam-google · 2017-03-14T07:11:11Z

Addressed review comments, PTAL.

src/main/java/com/google/api/gax/retrying/RetryFutureImpl.java

-        callbackFutureCallback.attemptFuture.cancel(mayInterruptIfRunning);
+        boolean rv = callbackFutureCallback.attemptFuture.cancel(mayInterruptIfRunning);
+        super.cancel(mayInterruptIfRunning);
+        return rv;


src/main/java/com/google/api/gax/retrying/AbstractRetryHandler.java

+    return totalTimeSpentNanos <= totalTimeoutNanos
+        && (globalSettings.getMaxAttempts() <= 0
+            || nextAttemptSettings.getAttemptCount() < globalSettings.getMaxAttempts());
+  }


src/main/java/com/google/api/gax/retrying/DirectRetryHandler.java

+      if (Duration.ZERO.compareTo(attemptSettings.getRandomizedRetryDelay()) < 0) {
+        Thread.sleep(attemptSettings.getRandomizedRetryDelay().getMillis());
+      }
+      return Futures.immediateFuture(callable.call());


src/main/java/com/google/api/gax/retrying/RetryAttemptSettings.java

+/**
+ * Execution attempt settings. Defines attempt-specific properties of a retry process.
+ */
+public class RetryAttemptSettings {


src/main/java/com/google/api/gax/retrying/RetryHandler.java

+   * @param attemptSettings current attempt settings
+   * @return the {@link Future}, representing the scheduled execution
+   */
+  Future<ResponseT> executeAttempt(


src/test/java/com/google/api/gax/grpc/RecordingScheduler.java

-  private final ScheduledExecutorService executor;
-  private final List<Duration> sleepDurations = new ArrayList<>();
-  private final FakeNanoClock clock;
+abstract class RecordingScheduler implements ScheduledExecutorService {


src/main/java/com/google/api/gax/core/RetrySettings.java

+     * this limit the logic will give up retrying even if the total retry time is still lower than
+     * TotalTimeout.
+     */
+    public abstract Builder setMaxAttempts(int maxAttempts);


… 2) time-specific and 3) exception-specific retry logics. 2. Removed abstract classes 3. Renamed NanoClock to ApiClock, as it has System.currentTimeMillis() implementation, and NanoClock name is misleading now.

vam-google · 2017-03-17T01:27:07Z

PTAL

Addressed the review comments, plus some other smaller changes:

Split RetryHandler on 3 different components: 1) scheduling logic, 2) time-specific and 3) exception-specific retry logics.
Removed abstract classes.
Renamed NanoClock to ApiClock, as it has System.currentTimeMillis() implementation, and NanoClock name is misleading now.

src/main/java/com/google/api/gax/core/NanoClock.java

+import java.util.concurrent.TimeUnit;
+
+/** Default implementation of the ApiClock interface, using call to System.nanoTime(). */
+public final class NanoClock implements ApiClock, Serializable {


src/main/java/com/google/api/gax/core/SystemClock.java

+ * Implementation of the {@link ApiClock} interface, which uses {@link System#currentTimeMillis()}
+ * as time source.
+ */
+public final class SystemClock implements ApiClock, Serializable {


src/main/java/com/google/api/gax/retrying/RetryingFutureImpl.java

+      }
+      if (attemptFuture != null) {
+        attemptFutureCallback = new AttemptFutureCallback(attemptFuture);
+        Futures.addCallback((ListenableFuture) attemptFuture, attemptFutureCallback);


src/main/java/com/google/api/gax/grpc/RetryingCallable.java

-          activeFuture = callFuture;
-        }
-      }
+      return null;


src/main/java/com/google/api/gax/retrying/ExceptionRetryAlgorithm.java

+   * Returns {@code true} if another attempt should be made, or {@code false} otherwise.
+   *
+   * @param prevThrowable exception thrown by the previous attempt
+   * @return {@code true} if another attempt should be made, or {@code false} otherwise


src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java

+  }
+
+  /**
+   * Creates a first attempt {@link TimedAttemptSettings}. By default the first attempt is


src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java

+      Throwable prevThrowable, TimedAttemptSettings prevSettings) {
+    TimedAttemptSettings newSettings =
+        exceptionAlgorithm.createNextAttempt(prevThrowable, prevSettings);
+    if (prevSettings.equals(newSettings)) {


src/main/java/com/google/api/gax/retrying/RetryingExecutor.java

+ *
+ * <ol>
+ * <li>Creating first attempt {@link RetryingFuture}, which acts as a facade, hiding from client
+ * code the actual scheduled retry attempts execution.


src/main/java/com/google/api/gax/retrying/RetryingExecutor.java

+   *
+   * @param retryingFuture the future previously returned by {@link #createFuture(Callable)}
+   */
+  void submit(RetryingFuture<ResponseT> retryingFuture);


shinfan

Looks good overall. Just a few more comments.

src/main/java/com/google/api/gax/core/RetrySettings.java

@@ -189,6 +206,13 @@ public Builder toBuilder() {
    public abstract double getRetryDelayMultiplier();

    /**
+     * MaxAttempts defines the maximum number of attempts to perform. If number of attempts reaches


src/main/java/com/google/api/gax/grpc/RetryingCallable.java

+      CallContext oldCtx, Duration rpcTimeout) {
+    CallOptions oldOpt = oldCtx.getCallOptions();
+    CallOptions newOpt = oldOpt.withDeadlineAfter(rpcTimeout.getMillis(), TimeUnit.MILLISECONDS);
+    CallContext newCtx = oldCtx.withCallOptions(newOpt);


src/main/java/com/google/api/gax/retrying/TimedAttemptSettings.java

+/**
+ * Timed attempt execution settings. Defines time-specific properties of a retry attempt.
+ */
+public class TimedAttemptSettings {


src/main/java/com/google/api/gax/retrying/ScheduledRetryingExecutor.java

+  }
+
+  /**
+   * Submits an attempt for execution in a different thread.


src/main/java/com/google/api/gax/grpc/RetryingCallable.java

+        new GrpcRetryCallable<>(callable, request, context);
+
+    RetryingFuture<ResponseT> retryingFuture = scheduler.createFuture(retryCallable);
+    retryCallable.setExternalFuture(retryingFuture);


vam-google · 2017-03-17T21:29:49Z

Adressed PR comments, PTAL.

vam-google · 2017-03-17T22:28:53Z

PTAL

garrettjonesgoogle · 2017-03-17T23:05:37Z

Offline agreements:

Rename SystemClock to CurrentMillisClock
Have ExceptionAlgorithm.createNextAttempt return null if it is not overriding the settings

LGTM after these are done - Shin should probably also approve.

…e next attempt settings. renamed SystemClock to CurrentMillisClock.

…tryAlgorithm, which now returns null)

vam-google · 2017-03-17T23:08:59Z

PTAL

shinfan

Just one more comment

src/main/java/com/google/api/gax/retrying/ScheduledRetryingExecutor.java

+
+/**
+ * The retry executor which uses {@link ScheduledExecutorService} to schedule an attempt tasks.
+ * Unless a direct executor service is used, this handler will schedule attempts for an execution in


vam-google · 2017-03-17T23:21:36Z

Done

shinfan · 2017-03-17T23:24:30Z

LGTM

Reconcile RetrySettings & Clock

fc30507

Reconcile RetrySettings in GAX with RetryParams in google-cloud-java googleapis/google-cloud-java#1574 Reconcile NanoClock in GAX with Clock in google-cloud-java googleapis/google-cloud-java#1575

vam-google assigned shinfan and garrettjonesgoogle Mar 11, 2017

Removed System.out.println

b31ce2b

vam-google added 2 commits March 10, 2017 17:50

Removed unused code, increased test coverage.

922bdf5

Fixed some documentation grammatical mistakes.

9756895

garrettjonesgoogle reviewed Mar 13, 2017

View reviewed changes

shinfan suggested changes Mar 13, 2017

View reviewed changes

Addressed review comments, PTAL.

e8d0c40

reverted versions back to 0.3.1-SNAPSHOT

eeb49b2

garrettjonesgoogle reviewed Mar 14, 2017

View reviewed changes

renamed rv variable

8da0218

shinfan reviewed Mar 15, 2017

View reviewed changes

vam-google added 2 commits March 16, 2017 17:38

Merge remote-tracking branch 'upstream/master'

583c1c4

1. Split RetryHandler on 3 different components: 1) scheduling logic,…

ff0ff36

… 2) time-specific and 3) exception-specific retry logics. 2. Removed abstract classes 3. Renamed NanoClock to ApiClock, as it has System.currentTimeMillis() implementation, and NanoClock name is misleading now.

googleJavaFormat files

b380e16

garrettjonesgoogle reviewed Mar 17, 2017

View reviewed changes

shinfan reviewed Mar 17, 2017

View reviewed changes

Adressed PR comments, PTAL.

9d79b03

vam-google added 2 commits March 17, 2017 16:06

changed retry algorithms to return null in case if they do not provid…

04f7f09

…e next attempt settings. renamed SystemClock to CurrentMillisClock.

fixed the test (failed because change of the contract for ExceptionRe…

d3eabec

…tryAlgorithm, which now returns null)

shinfan approved these changes Mar 17, 2017

View reviewed changes

updated documentation for ScheduledExcecutorService

87caffd

vam-google added 3 commits March 20, 2017 11:13

Merge remote-tracking branch 'upstream/master'

faad8e3

udated javadoc for ApiClock

7500a2d

Updated docs and formatting

db34591

vam-google merged commit dfb7021 into googleapis:master Mar 20, 2017

		@@ -0,0 +1,61 @@
		/*
		* Copyright 2016, Google Inc. All rights reserved.

Reconcile RetrySettings & Clock #235

Reconcile RetrySettings & Clock #235

Conversation

vam-google commented Mar 11, 2017

codecov-io commented Mar 11, 2017 • edited Loading

Codecov Report

garrettjonesgoogle left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

vam-google commented Mar 14, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

vam-google commented Mar 17, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

shinfan left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

codecov-io commented Mar 11, 2017 •

edited

Loading