Add RetryingContext #590

igorbernstein2 · 2018-10-05T00:59:33Z

This is the next iteration of #583.

This is the first step of opencensus integration. To enable tracing I need to inject state into BasicRetryingFuture#handleAttempt, unfortunately as it stands RetryingExecutor and thus RetryingFutures don't have access to per-operation state. This prevents retrying logic from annotating the current operation span. This PR remedies the situation by injecting state via a RetryingContext that is passed toRetryingExecutor#createFuture.
Unfortunately RetryingExecutor interface is part of the public surface that has been finalized. To sidestep this issue, a new interface RetryingExecutorWithContext is introduced that inherits from RetryingExecutor.
RetryingExecutorWithContext is meant to be a temporary solution. Once gax drops support for java 7, the new method can be pulled up to the RetryingExecutor interface with a a default implementation.

This is the first step in implementing opencensus integration. It allows me to inject operation scoped state into the retrying logic

codecov-io · 2018-10-05T01:05:08Z

Codecov Report

Merging #590 into master will decrease coverage by 0.06%.
The diff coverage is 68.75%.

@@             Coverage Diff              @@
##             master     #590      +/-   ##
============================================
- Coverage     75.09%   75.03%   -0.07%     
  Complexity      935      935              
============================================
  Files           176      177       +1     
  Lines          4084     4090       +6     
  Branches        323      323              
============================================
+ Hits           3067     3069       +2     
- Misses          865      869       +4     
  Partials        152      152

Impacted Files	Coverage Δ	Complexity Δ
.../com/google/api/gax/rpc/OperationCallableImpl.java	`100% <ø> (ø)`	`5 <0> (ø)`	⬇️
...e/api/gax/rpc/RetryingServerStreamingCallable.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...m/google/api/gax/retrying/NoopRetryingContext.java	`0% <0%> (ø)`	`0 <0> (?)`
...rc/main/java/com/google/api/gax/rpc/Callables.java	`68.18% <100%> (ø)`	`7 <0> (ø)`	⬇️
.../api/gax/retrying/CallbackChainRetryingFuture.java	`88.37% <100%> (ø)`	`6 <1> (ø)`	⬇️
...ava/com/google/api/gax/rpc/RecheckingCallable.java	`90.9% <100%> (+0.9%)`	`2 <1> (ø)`	⬇️
...m/google/api/gax/retrying/BasicRetryingFuture.java	`92.85% <100%> (+0.08%)`	`22 <0> (ø)`	⬇️
.../java/com/google/api/gax/rpc/RetryingCallable.java	`91.66% <100%> (ø)`	`2 <1> (ø)`	⬇️
...oogle/api/gax/retrying/DirectRetryingExecutor.java	`78.94% <50%> (-4.39%)`	`6 <1> (ø)`
...le/api/gax/retrying/ScheduledRetryingExecutor.java	`92.3% <50%> (-7.7%)`	`3 <1> (ø)`
... and 1 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 7b19a10...af756a2. Read the comment docs.

igorbernstein2 · 2018-10-05T02:07:48Z

@vam-google, @garrettjonesgoogle please take a look when you have some time

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

+@BetaApi
+@InternalExtensionOnly
+@AutoValue
+public abstract class RetryingContext {


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

 */
 class CallbackChainRetryingFuture<ResponseT> extends BasicRetryingFuture<ResponseT> {
-  private final RetryingExecutor<ResponseT> retryingExecutor;
+  private final ScheduledRetryingExecutor<ResponseT> retryingExecutor;


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

+  @BetaApi("The surface for passing per operation state is not yet stable")
+  @Override
+  public RetryingFuture<ResponseT> createFuture(
+      @Nonnull Callable<ResponseT> callable, @Nonnull RetryingContext retryingContext) {


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

+ * <p>It provides state to individual {@link RetryingFuture}s via the {@link RetryingExecutor}.
+ */
+@BetaApi
+@InternalExtensionOnly


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

+    return newBuilder().build();
+  }
+
+  public static RetryingContext fromCallContext(@Nullable ApiCallContext callContext) {


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

+  }
+
+  @BetaApi
+  @InternalExtensionOnly


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

+/**
+ * A retrying executor is responsible for the following operations:
+ *
+ * <ol>


gax/src/test/java/com/google/api/gax/retrying/DirectRetryingExecutorTest.java

  }
+
+  @Test
+  public void testFutureContainsRetryContext() {


gax/src/test/java/com/google/api/gax/retrying/DirectRetryingExecutorTest.java

+    BasicRetryingFuture<String> future =
+        (BasicRetryingFuture<String>) executor.createFuture(noopCallable, ctx);
+
+    Truth.assertThat(future.getRetryingContext()).isSameAs(ctx);


igorbernstein2 · 2018-10-09T18:11:52Z

I've responded to all feedback. PTAL

igorbernstein2 · 2018-10-09T20:13:35Z

All feedback should be addressed. PTAL

vam-google

Only one comment, but I believe it is essential.

gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallContext.java

+  @Override
+  public RetryingContext getRetryContext() {
+    // TODO: transfer relevant state from the ApiCallContext
+    return RetryingContext.newBuilder().build();


igorbernstein · 2018-10-11T13:39:41Z

Thinking about this further, both of the attributes (tracer and overallTimeout) that are needed by RetryContext belong on the ApiCallContext itself:

the tracer will need to be accessible to other parts of the codebase to annotate connections, annotate stream progress, start actual operation spans, etc
the overallTimeout is not only about retries and will take effect on non-retryable rpcs (it will be the minimum of the rpc timeout and the overall timeout). This allows the caller to state their intention at the callsite ("I have to respond to the ad auction within 10 ms, so I need to make sure the bigtable rpc doesn't take longer than 5 ms") without digging into the client implementation to see if retries are enabled for that rpc.

Duplicating these attributes in the RetryContext was done to avoid having the retrying package import classes from the rpc package.

benchmark/build.gradle

@@ -1,6 +1,5 @@
 buildscript {
    repositories {
-        jcenter()


…ontext

igorbernstein2 · 2018-10-12T15:55:58Z

As discussed offline, RetryingContext is now an interface that ApiCallContext extends. PTAL

vam-google

LGTM, but please consider renaming retryingContext (as parameter) to just context, since "retrying" prefix is redundant here.

igorbernstein2 · 2018-10-15T18:52:02Z

Thanks for the review. I dropped the prefix

igorbernstein2 added 3 commits October 4, 2018 16:35

WIP: add an empty RetryingContext

c17c139

This is the first step in implementing opencensus integration. It allows me to inject operation scoped state into the retrying logic

more plumbing

5fdd7af

more work

209a841

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 5, 2018

igorbernstein2 mentioned this pull request Oct 5, 2018

RFC: rough sketch of retry tracing (DON'T MERGE) #583

Closed

garrettjonesgoogle requested a review from vam-google October 5, 2018 16:53

garrettjonesgoogle reviewed Oct 5, 2018

View reviewed changes

igorbernstein2 changed the title ~~WIP: Add RetryingContext~~ Add RetryingContext Oct 5, 2018

vam-google reviewed Oct 5, 2018

View reviewed changes

update docs

7ae5903

googleapis deleted a comment from igorbernstein2 Oct 9, 2018

igorbernstein2 added 3 commits October 9, 2018 16:05

remove nonnull annotations

f354b17

move retrycontext construction to apicallcontext subclasses

0fea88e

remove tests

de45791

vam-google reviewed Oct 10, 2018

View reviewed changes

igorbernstein2 added 2 commits October 11, 2018 12:05

empty commit to trigger ci

89b090f

remove jcenter repo for benchmark gradle plugins

75dc914

igorbernstein2 commented Oct 11, 2018

View reviewed changes

benchmark/build.gradle Outdated

@@ -1,6 +1,5 @@

buildscript {

repositories {

jcenter()

This comment was marked as spam.

Sign in to view

igorbernstein2 added 5 commits October 11, 2018 14:44

Merge remote-tracking branch 'origin/master' into opencensus-0-retryc…

064e2f7

…ontext

change RetryingContext into an interface that ApiCallContext extends

f980242

Merge remote-tracking branch 'origin/master' into opencensus-0-retryc…

58a6d1b

…ontext

Make it nonnull

96638dc

tweaks

09c69b8

vam-google approved these changes Oct 15, 2018

View reviewed changes

s/retryingContext/context

af756a2

igorbernstein2 merged commit 90957ff into googleapis:master Oct 15, 2018

igorbernstein2 deleted the opencensus-0-retrycontext branch October 15, 2018 18:52

garrettjonesgoogle mentioned this pull request Oct 19, 2018

Release gax-java v1.34.0 #611

Merged

Raibaz pushed a commit to Raibaz/gax-java that referenced this pull request Oct 25, 2018

Add RetryingContext (googleapis#590)

8403703

Raibaz pushed a commit to Raibaz/gax-java that referenced this pull request Oct 25, 2018

Add RetryingContext (googleapis#590)

a598db8

igorbernstein2 mentioned this pull request May 14, 2019

Broken setSimpleTimeoutNoRetries() #711

Closed

alicejli mentioned this pull request Mar 27, 2024

gax: Consider removing @BetaApi and/or adding @InternalExtensionOnly to retry surface googleapis/sdk-platform-java#2096

Open

Add RetryingContext #590

Add RetryingContext #590

Uh oh!

Conversation

igorbernstein2 commented Oct 5, 2018

Uh oh!

codecov-io commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

igorbernstein2 commented Oct 5, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

codecov-io commented Oct 5, 2018 •

edited

Loading

igorbernstein commented Oct 11, 2018 •

edited by igorbernstein2

Loading