feat: support retry settings and retryable codes in call context #1238

olavloite · 2020-11-09T17:53:40Z

Adds support for retry settings and retryable codes for individual RPCs through ApiCallContext.

Example usage:

ApiCallContext context = GrpcCallContext.createDefault()
  .withRetrySettings(RetrySettings.newBuilder()
    .setInitialRetryDelay(Duration.ofMillis(10L))
    .setInitialRpcTimeout(Duration.ofMillis(100L))
    .setMaxAttempts(10)
    .setMaxRetryDelay(Duration.ofSeconds(10L))
    .setMaxRpcTimeout(Duration.ofSeconds(30L))
    .setRetryDelayMultiplier(1.4)
    .setRpcTimeoutMultiplier(1.5)
    .setTotalTimeout(Duration.ofMinutes(10L))
    .build())
  .withRetryableCodes(Sets.newSet(
    StatusCode.Code.UNAVAILABLE,
    StatusCode.Code.DEADLINE_EXCEEDED));

Points for consideration:

This feature can only be used for RPC's that are already retryable in the default settings, as the current code does the following optimization:

gax-java/gax/src/main/java/com/google/api/gax/rpc/Callables.java

Line 86 in aabd3b0

if (areRetriesDisabled(callSettings.getRetryableCodes(), callSettings.getRetrySettings())) {

Calling withRetrySettings(...) or withRetryableCodes(...) will not change the retry behavior of those RPC's.
This PR introduces a number of ContextAware... classes that accept a RetryingContext as an argument for the methods that determine whether to retry an RPC, and if so, with what settings. This ensures that this PR is not a breaking change. It would also be possible to add this behavior to existing classes, but that would mean:
2.1. It would be a breaking change
2.2. OR it would include multiple if (someAlgorithm instanceof ContextAwareAlgorithm) return ((ContextAwareAlgorithm) algorithm).shouldRetry(context, ...); type statements in the existing classes to check whether it has been passed an algorithm that is able to determine retry settings using a RetryingContext or not.

Fixes #1197

Allows applications to supply retry settings and retryabe codes for individual RPCs through ApiCallContext. Fixes googleapis#1197

codecov · 2020-11-09T19:55:54Z

Codecov Report

Merging #1238 (41fdc4e) into master (86d5c72) will decrease coverage by 0.15%.
The diff coverage is 84.76%.

❗ Current head 41fdc4e differs from pull request most recent head 8eab974. Consider uploading reports for the commit 8eab974 to get more accurate results

@@             Coverage Diff              @@
##             master    #1238      +/-   ##
============================================
- Coverage     79.65%   79.49%   -0.16%     
- Complexity     1249     1291      +42     
============================================
  Files           209      215       +6     
  Lines          5461     5516      +55     
  Branches        464      472       +8     
============================================
+ Hits           4350     4385      +35     
- Misses          928      945      +17     
- Partials        183      186       +3

Impacted Files	Coverage Δ	Complexity Δ
...m/google/api/gax/retrying/NoopRetryingContext.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...va/com/google/api/gax/retrying/RetryAlgorithm.java	`87.50% <ø> (+6.25%)`	`9.00 <0.00> (+1.00)`
...rc/main/java/com/google/api/gax/rpc/Callables.java	`68.62% <50.00%> (ø)`	`12.00 <0.00> (ø)`
...m/google/api/gax/httpjson/HttpJsonCallContext.java	`70.00% <68.42%> (+0.58%)`	`33.00 <6.00> (+5.00)`
.../java/com/google/api/gax/grpc/GrpcCallContext.java	`84.17% <72.22%> (-1.43%)`	`53.00 <6.00> (+5.00)`	⬇️
.../gax/retrying/ContextAwareBasicRetryingFuture.java	`87.50% <87.50%> (ø)`	`5.00 <5.00> (?)`
...rying/ContextAwareCallbackChainRetryingFuture.java	`87.50% <87.50%> (ø)`	`5.00 <5.00> (?)`
...m/google/api/gax/retrying/BasicRetryingFuture.java	`92.59% <88.88%> (-0.55%)`	`29.00 <4.00> (+4.00)`	⬇️
...i/gax/rpc/ContextAwareStreamingRetryAlgorithm.java	`89.47% <89.47%> (ø)`	`5.00 <5.00> (?)`
...e/api/gax/retrying/ContextAwareRetryAlgorithm.java	`93.33% <93.33%> (ø)`	`10.00 <10.00> (?)`
... and 26 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 86d5c72...8eab974. Read the comment docs.

olavloite · 2021-01-03T19:18:45Z

@igorbernstein2 Friendly ping on this PR. Would you mind taking a look? If you're not the best person to weigh in on this, then please let me know.

cc @skuruppu

noahdietz · 2021-01-05T23:32:34Z

This looks great (to my untrained Java eye). I'd like @igorbernstein2 and/or @vam-google / @miraleung to review it though, they are the more Java savvy and experienced in this codebase.

Of course, @chingor13 should be approve of the added functionality (I already polled these folks prior to starting this work, but just for posterity...).

elharo · 2021-01-06T12:12:48Z

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

    this.channel = channel;
    this.callOptions = Preconditions.checkNotNull(callOptions);
    this.timeout = timeout;
    this.streamWaitTimeout = streamWaitTimeout;
    this.streamIdleTimeout = streamIdleTimeout;
    this.channelAffinity = channelAffinity;
    this.extraHeaders = Preconditions.checkNotNull(extraHeaders);
+    this.retrySettings = retrySettings;
+    this.retryableCodes = retryableCodes == null ? null : ImmutableSet.copyOf(retryableCodes);


Why is this nullable? per Effective Java, empty collections are preferred.

There is a difference in this case:

null means 'do not override the default settings`.

An empty set means 'override the default settings by disabling retrying'.

I've added that to the documentation of ApiCallContext#getRetryableCodes().

gax-grpc/src/test/java/com/google/api/gax/grpc/GrpcCallContextTest.java

gax/src/test/java/com/google/api/gax/rpc/ApiResultRetryAlgorithmTest.java

gax/src/test/java/com/google/api/gax/rpc/RetryingTest.java

elharo

Where do the docs for retries live? Does something need to be added to package-info?

elharo

Google policy is that existing violations of the style guide do not justify further violations. All new code must follow the style guide, irrespective of existing code.

olavloite · 2021-01-07T13:27:09Z

@elharo Thanks for the review. I (think I) have addressed all your comments. PTAL.

olavloite · 2021-01-19T08:51:07Z

@elharo Friendly ping on this PR. I addressed your comments, would you mind taking a second look?

vam-google

Looks good with few (hopefully last ones) comments. Basically I still hope we can get rid of the Ignore* classes.

Also, optionally, please consider having some clear strategy for all of the context-aware and context-unaware methods:

Make context-unaware methods deprecated,
make sure that one implementation always calls another one to avoid code duplication (if context-unaware implementaitons got deprecated then it should be context-unaware call context-aware, with context probably as null)
make sure that methods documentation reference each other and explains the situation (that both methods are the same thing, and context-awareness is the only difference)

vam-google · 2021-03-08T19:48:35Z

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

+ * RetryingContext}. This is used to wrap {@link ResultRetryAlgorithm} instances to create a {@link
+ * ResultRetryAlgorithmWithContext} when one is required.
+ */
+class IgnoreRetryingContextResultRetryAlgorithm<ResponseT>


The way the new constructor is implemented in RetryAlgorithm (the one which implementation you listed in the comment) seems being kind of against OOP principles, since the constructor does accept a generic interface, but then instantiates the specific instances of the interface instead. I understand that sometimes we may need to do something like this, but I'm not sure it is that case.

Looks like the Ignore* classes exist only because RetryAlgorithm implementation changed the aggregated fields type to the context-aware interfaces:

private final ResultRetryAlgorithmWithContext<ResponseT> resultAlgorithm; private final TimedRetryAlgorithmWithContext timedAlgorithm;

A the same time this PR adds the full copy of all the existing methods in RetryAlgorithm but with the context extra parameter now. I.e. this means that this class can be used eitehr as context-aware one or as context-unaware, but not in both different context. WDYT about instead of adding the new Ignore* classes, just make RetryAlgorithm just have both types of algorithms aggregated:

private final ResultRetryAlgorithm<ResponseT> resultAlgorithm; private final TimedRetryAlgorithm timedAlgorithm; private final ResultRetryAlgorithmWithContext<ResponseT> resultAlgorithmWithContext; private final TimedRetryAlgorithmWithContext timedAlgorithmWithContext;

And use each of the algorithms accordingly depending in which method of REtryAlgorithm class it is used (context-aware or not).

In general, I'm just trying to avoid having classes like IgnoreRetryingContextReulstRetryAlgorithm because they are obviously a hack (even check the name, it is too long an unreadable). So here I'm trying to trade it to simply having context-aware fields as null for older constructor, and it is ok to throw an exception if context-aware methods are used for RetryAlgorithm initialized in legacy way.

Also, as you suggest in your comments, please go ahead and deprecate the old constructor for RetryAlgorithm and relevant stuff (maybe even the old context-unaware methods as well).

vam-google · 2021-03-08T20:28:02Z

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

+   * @param previousResponse response returned by the previous attempt
+   * @param previousSettings previous attempt settings
+   */
+  public TimedAttemptSettings createNextAttempt(


Please don't use access modifiers (public) on interface methods, as they are redundant.

vam-google · 2021-03-08T20:33:16Z

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

+      Throwable previousThrowable,
+      ResponseT previousResponse,
+      TimedAttemptSettings previousSettings) {
+    return null;


can you call createNextAttempt() (no context one) here for consistency with the shouldRetry implementation?

vam-google · 2021-03-08T20:38:53Z

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

+    extends ResultRetryAlgorithm<ResponseT> {
+
+  /**
+   * Creates a next attempt {@link TimedAttemptSettings}.


Optional (but please consider doing it). It seems like a lot of method duplication is happening here (context-aware vs context-unaware methods). Please consider replacing documentation for every context-unaware method in all of the classes with a link to the context-aware one with the explanation, that this one (context-unaware one) is doing the same but without the context. This will not only can make it shorter in terms of docs, but will also provide a clear picture to the users of these classes why there are both types of methods (which may be really confusing).

Also, please consider deprecating the context-unaware methods.

I've updated the documentation of the context-unaware interfaces and their methods to reference the context-aware versions.

I've deprecated all the context-unaware methods

vam-google · 2021-03-08T20:41:45Z

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

+   * <p>The attempt settings will be reset if the stream attempt produced any messages.
+   */
+  @Override
+  public TimedAttemptSettings createNextAttempt(


Can we make these methods (context-aware and context-unaware methods in streaming algorithm) call each other, like it is done in the non-streaming versions? This is to reduce code dupliation and make the implementation more robust in general.

Done (this is now possible as the context-aware methods accept null values for the context)

vam-google · 2021-03-08T20:54:49Z

gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java

+   *     StatusCode.Code.DEADLINE_EXCEEDED));
+   * }</pre>
+   */
+  ApiCallContext withRetrySettings(RetrySettings retrySettings);


Can we mark these new methods as BetaApi. This an addition to a fundamental interface, whatever we add here we will have to stick with for a really long time.

gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java

…a into context-retry-settings

vam-google

LGTM, but please cleanup the deprecation stuff (more details in the comment). Sorry for confusing with it.

vam-google · 2021-03-11T23:35:24Z

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

   *     thrown instead
   * @param nextAttemptSettings attempt settings, which will be used for the next attempt, if
   *     accepted
   * @throws CancellationException if the retrying process should be canceled
   * @return {@code true} if another attempt should be made, or {@code false} otherwise
   */
  public boolean shouldRetry(
-      Throwable prevThrowable, ResponseT prevResponse, TimedAttemptSettings nextAttemptSettings)
+      Throwable previousThrowable,


Sorry for confusing with my @Deprecate request. I think we should just mark the methods and constructors in RetryAlgorithm class as deprecated (the old ones) and make sure that the non-deprecated methos (new ones) are called by the deprecated methods (which is what is happening now, so it is all good), otherwise we will be using our own deprecated methods to implement non-deprecated methods, which is wrong.

For the rest of it, please undeprecate the methods back, as is is clear that RetryAlgorithm cannot be used in both contexts (as context-aware and as unaware), but the actual interfaces (timed and result) kind of still potentially can have meaningful implementation for all the methods, but most importantly it is too much deprecation happening on interface level, I'm afraid we are going to get too many deprecaton warnings.

So to summarize: deprecate the old constructor and methods in RetryAlgorithm class, undeprecate the other stuff (the TimedRetryAlgorithm and ResultRetryAlgorithm methods - in both places, the @Deprecated annotation and @deprecated java doc element, maybe adding in the general java doc section of each element a notion that the other method should be preferred, but it does not mean it is deprecated in a strict sense of it).

noahdietz · 2021-03-16T16:43:40Z

@olavloite heads up #1328 we are wrapping all RPC callables in the retry machinery which should enable the use of this feature on non-retryable RPCs once merged.

Note: #1328 has the same numbers as this PR 1238, how about that! :)

noahdietz · 2021-03-16T21:19:04Z

@miraleung can you review/approve this PR again? You requested changes at one point.

olavloite · 2021-03-17T10:14:25Z

@olavloite heads up #1328 we are wrapping all RPC callables in the retry machinery which should enable the use of this feature on non-retryable RPCs once merged.

Note: #1328 has the same numbers as this PR 1238, how about that! :)

Cool (both the feature and the numbers) :-)

noahdietz · 2021-03-17T20:44:36Z

@olavloite heads up #1328 we are wrapping all RPC callables in the retry machinery which should enable the use of this feature on non-retryable RPCs once merged.
Note: #1328 has the same numbers as this PR 1238, how about that! :)

Cool (both the feature and the numbers) :-)

BTW @olavloite if this is good to go, feel free to merge this - no need to wait on the callable wrapping change I mentioned.

miraleung

LGTM, please wait for others to LGTM as well.

noahdietz

LGTM this is really awesome, Knut

noahdietz · 2021-03-17T20:53:10Z

Removed @igorbernstein2 because we had thorough reviews from others.

feat: support retry settings and retryable codes in call context

8eeefdc

Allows applications to supply retry settings and retryabe codes for individual RPCs through ApiCallContext. Fixes googleapis#1197

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 9, 2020

chore: fix formatting and license headers

83c2f16

feat: use context retry settings for streaming calls

aabd3b0

olavloite changed the title ~~[WIP] feat: support retry settings and retryable codes in call context~~ feat: support retry settings and retryable codes in call context Nov 11, 2020

Merge branch 'master' into context-retry-settings

d73e7b3

olavloite marked this pull request as ready for review November 11, 2020 19:31

olavloite requested review from a team as code owners November 11, 2020 19:31

olavloite requested review from noahdietz, igorbernstein2 and vam-google November 11, 2020 19:31

Merge branch 'master' into context-retry-settings

c58f9c4

elharo reviewed Jan 6, 2021

View reviewed changes

olavloite added 2 commits January 7, 2021 12:29

fix: process review comments

ed22eaf

fix: missed one Thread.sleep

80230e1

elharo suggested changes Jan 7, 2021

View reviewed changes

olavloite added 2 commits January 7, 2021 14:12

fix: fix style

8ef6e6f

Merge branch 'master' into context-retry-settings

19ed8d0

olavloite requested a review from elharo January 7, 2021 13:26

Merge branch 'master' into context-retry-settings

b2999d6

Merge branch 'master' into context-retry-settings

3eb9fa7

miraleung self-requested a review February 4, 2021 18:39

Merge branch 'master' into context-retry-settings

73b26c7

vam-google reviewed Mar 8, 2021

View reviewed changes

noahdietz reviewed Mar 9, 2021

View reviewed changes

gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java Outdated Show resolved Hide resolved

noahdietz reviewed Mar 9, 2021

View reviewed changes

gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java Outdated Show resolved Hide resolved

noahdietz mentioned this pull request Mar 9, 2021

Should global timeout override timeout in provided context? #1144

Closed

olavloite added 7 commits March 10, 2021 17:17

fix: address review comments

e02f951

Merge branch 'context-retry-settings' of github.com:olavloite/gax-jav…

a0118c3

…a into context-retry-settings

fix: add tests + fix potential NPE

eafe239

Merge branch 'master' into context-retry-settings

ffa05a1

fix: remove unused method + add tests

8ae3280

Merge branch 'context-retry-settings' of github.com:olavloite/gax-jav…

3c0d719

…a into context-retry-settings

test: add additional test calls

5da364a

olavloite requested a review from vam-google March 10, 2021 19:10

noahdietz mentioned this pull request Mar 11, 2021

[gax] wrap non-retryable RPCs in the same retryable callable machinery #1327

Closed

vam-google approved these changes Mar 11, 2021

View reviewed changes

noahdietz mentioned this pull request Mar 12, 2021

feat: wrap non-retryable RPCs in retry machinery #1328

Merged

olavloite added 2 commits March 17, 2021 11:13

fix: deprecation

a42ea60

Merge branch 'master' into context-retry-settings

8eab974

miraleung approved these changes Mar 17, 2021

View reviewed changes

noahdietz approved these changes Mar 17, 2021

View reviewed changes

noahdietz removed the request for review from igorbernstein2 March 17, 2021 20:52

olavloite merged commit 7f7aa25 into googleapis:master Mar 18, 2021

olavloite deleted the context-retry-settings branch March 18, 2021 08:25

noahdietz mentioned this pull request Mar 23, 2021

[ggj] generate example of ApiCallContext.withRetrySettings on client googleapis/sdk-platform-java#700

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support retry settings and retryable codes in call context #1238

feat: support retry settings and retryable codes in call context #1238

olavloite commented Nov 9, 2020 •

edited

codecov bot commented Nov 9, 2020 •

edited

olavloite commented Jan 3, 2021

noahdietz commented Jan 5, 2021

elharo Jan 6, 2021

olavloite Jan 7, 2021

elharo left a comment

elharo left a comment

olavloite commented Jan 7, 2021

olavloite commented Jan 19, 2021

vam-google left a comment

vam-google Mar 8, 2021

vam-google Mar 8, 2021

vam-google Mar 8, 2021

vam-google Mar 8, 2021

olavloite Mar 10, 2021

vam-google Mar 8, 2021

olavloite Mar 10, 2021

vam-google Mar 8, 2021

olavloite Mar 10, 2021

vam-google left a comment

vam-google Mar 11, 2021 •

edited

olavloite Mar 17, 2021

noahdietz commented Mar 16, 2021

noahdietz commented Mar 16, 2021

olavloite commented Mar 17, 2021

noahdietz commented Mar 17, 2021 •

edited

miraleung left a comment

noahdietz left a comment

noahdietz commented Mar 17, 2021

feat: support retry settings and retryable codes in call context #1238

feat: support retry settings and retryable codes in call context #1238

Conversation

olavloite commented Nov 9, 2020 • edited

codecov bot commented Nov 9, 2020 • edited

Codecov Report

olavloite commented Jan 3, 2021

noahdietz commented Jan 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

olavloite commented Jan 7, 2021

olavloite commented Jan 19, 2021

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

vam-google Mar 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahdietz commented Mar 16, 2021

noahdietz commented Mar 16, 2021

olavloite commented Mar 17, 2021

noahdietz commented Mar 17, 2021 • edited

miraleung left a comment

Choose a reason for hiding this comment

noahdietz left a comment

Choose a reason for hiding this comment

noahdietz commented Mar 17, 2021

olavloite commented Nov 9, 2020 •

edited

codecov bot commented Nov 9, 2020 •

edited

vam-google Mar 11, 2021 •

edited

noahdietz commented Mar 17, 2021 •

edited