Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

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

Merged
merged 37 commits into from Mar 18, 2021
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
8eeefdc
feat: support retry settings and retryable codes in call context
olavloite Nov 9, 2020
83c2f16
chore: fix formatting and license headers
olavloite Nov 9, 2020
aabd3b0
feat: use context retry settings for streaming calls
olavloite Nov 11, 2020
d73e7b3
Merge branch 'master' into context-retry-settings
olavloite Nov 11, 2020
c58f9c4
Merge branch 'master' into context-retry-settings
olavloite Jan 3, 2021
ed22eaf
fix: process review comments
olavloite Jan 7, 2021
80230e1
fix: missed one Thread.sleep
olavloite Jan 7, 2021
8ef6e6f
fix: fix style
olavloite Jan 7, 2021
19ed8d0
Merge branch 'master' into context-retry-settings
olavloite Jan 7, 2021
b2999d6
Merge branch 'master' into context-retry-settings
olavloite Jan 19, 2021
3eb9fa7
Merge branch 'master' into context-retry-settings
olavloite Jan 26, 2021
73b26c7
Merge branch 'master' into context-retry-settings
olavloite Feb 4, 2021
111f8f5
fix: address review comments
olavloite Feb 5, 2021
a58eb10
refactor: remove ContextAwareResultRetryAlgorithm
olavloite Feb 5, 2021
51b670b
refactor: remove ContextAwareTimedRetryAlgorithm
olavloite Feb 5, 2021
41fdc4e
fix: make package-private + add ignored
olavloite Feb 5, 2021
68b2c69
revert: revert to always using global settings for jittered
olavloite Feb 5, 2021
f6beeef
test: add tests for NoopRetryingContext
olavloite Feb 8, 2021
54f9f70
Merge branch 'master' into context-retry-settings
olavloite Feb 22, 2021
da9cc81
Merge branch 'master' into context-retry-settings
olavloite Feb 24, 2021
8c5090d
chore: cleanup 'this' usage
olavloite Feb 24, 2021
131da46
Merge branch 'master' into context-retry-settings
olavloite Mar 1, 2021
395d7cf
refactor: merge context aware classes with base classes
olavloite Mar 1, 2021
5c90e70
chore: cleanup and remove unnecessary methods
olavloite Mar 1, 2021
301069d
test: add safety margin as the retry settings are now jittered
olavloite Mar 1, 2021
f322549
docs: add javadoc
olavloite Mar 1, 2021
e0a61c5
chore: address review comments
olavloite Mar 5, 2021
0925ea2
Merge branch 'master' into context-retry-settings
olavloite Mar 5, 2021
e02f951
fix: address review comments
olavloite Mar 10, 2021
a0118c3
Merge branch 'context-retry-settings' of github.com:olavloite/gax-jav…
olavloite Mar 10, 2021
eafe239
fix: add tests + fix potential NPE
olavloite Mar 10, 2021
ffa05a1
Merge branch 'master' into context-retry-settings
olavloite Mar 10, 2021
8ae3280
fix: remove unused method + add tests
olavloite Mar 10, 2021
3c0d719
Merge branch 'context-retry-settings' of github.com:olavloite/gax-jav…
olavloite Mar 10, 2021
5da364a
test: add additional test calls
olavloite Mar 10, 2021
a42ea60
fix: deprecation
olavloite Mar 17, 2021
8eab974
Merge branch 'master' into context-retry-settings
olavloite Mar 17, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
139 changes: 118 additions & 21 deletions gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallContext.java
Expand Up @@ -29,16 +29,19 @@
*/
package com.google.api.gax.grpc;

import com.google.api.core.BetaApi;
import com.google.api.core.InternalExtensionOnly;
import com.google.api.gax.retrying.RetrySettings;
import com.google.api.gax.rpc.ApiCallContext;
import com.google.api.gax.rpc.StatusCode;
import com.google.api.gax.rpc.TransportChannel;
import com.google.api.gax.rpc.internal.Headers;
import com.google.api.gax.tracing.ApiTracer;
import com.google.api.gax.tracing.NoopApiTracer;
import com.google.auth.Credentials;
import com.google.common.annotations.Beta;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is defined in guava, guava has been explicitly shaded from the gax surface. @BetaApi annotation must be used instead, as it is used in the rest of the gax.

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import io.grpc.CallCredentials;
import io.grpc.CallOptions;
import io.grpc.CallOptions.Key;
Expand All @@ -49,6 +52,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.threeten.bp.Duration;
Expand All @@ -60,8 +64,11 @@
* GrpcCallContext itself or the underlying data. Methods of the form {@code withX}, such as {@link
* #withTransportChannel}, return copies of the object, but with one field changed. The immutability
* and thread safety of the arguments solely depends on the arguments themselves.
*
* <p>Applications should reference {@link ApiCallContext} instead. This class is likely to
* experience breaking changes.
*/
@BetaApi("Reference ApiCallContext instead - this class is likely to experience breaking changes")
@Beta
@InternalExtensionOnly
public final class GrpcCallContext implements ApiCallContext {
static final CallOptions.Key<ApiTracer> TRACER_KEY = Key.create("gax.tracer");
Expand All @@ -72,18 +79,36 @@ public final class GrpcCallContext implements ApiCallContext {
@Nullable private final Duration streamWaitTimeout;
@Nullable private final Duration streamIdleTimeout;
@Nullable private final Integer channelAffinity;
@Nullable private final RetrySettings retrySettings;
@Nullable private final ImmutableSet<StatusCode.Code> retryableCodes;
private final ImmutableMap<String, List<String>> extraHeaders;

/** Returns an empty instance with a null channel and default {@link CallOptions}. */
public static GrpcCallContext createDefault() {
return new GrpcCallContext(
null, CallOptions.DEFAULT, null, null, null, null, ImmutableMap.<String, List<String>>of());
null,
CallOptions.DEFAULT,
null,
null,
null,
null,
ImmutableMap.<String, List<String>>of(),
null,
null);
}

/** Returns an instance with the given channel and {@link CallOptions}. */
public static GrpcCallContext of(Channel channel, CallOptions callOptions) {
return new GrpcCallContext(
channel, callOptions, null, null, null, null, ImmutableMap.<String, List<String>>of());
channel,
callOptions,
null,
null,
null,
null,
ImmutableMap.<String, List<String>>of(),
null,
null);
}

private GrpcCallContext(
Expand All @@ -93,14 +118,18 @@ private GrpcCallContext(
@Nullable Duration streamWaitTimeout,
@Nullable Duration streamIdleTimeout,
@Nullable Integer channelAffinity,
ImmutableMap<String, List<String>> extraHeaders) {
ImmutableMap<String, List<String>> extraHeaders,
@Nullable RetrySettings retrySettings,
@Nullable Set<StatusCode.Code> retryableCodes) {
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

}

/**
Expand Down Expand Up @@ -162,7 +191,9 @@ public GrpcCallContext withTimeout(@Nullable Duration timeout) {
this.streamWaitTimeout,
this.streamIdleTimeout,
this.channelAffinity,
this.extraHeaders);
this.extraHeaders,
retrySettings,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be this.retrySettings at least for the style consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, done (and cleaned up the other instances).

this.retryableCodes);
}

@Nullable
Expand All @@ -185,7 +216,9 @@ public GrpcCallContext withStreamWaitTimeout(@Nullable Duration streamWaitTimeou
streamWaitTimeout,
streamIdleTimeout,
channelAffinity,
extraHeaders);
extraHeaders,
retrySettings,
retryableCodes);
}

@Override
Expand All @@ -202,10 +235,12 @@ public GrpcCallContext withStreamIdleTimeout(@Nullable Duration streamIdleTimeou
streamWaitTimeout,
streamIdleTimeout,
channelAffinity,
extraHeaders);
extraHeaders,
retrySettings,
retryableCodes);
}

@BetaApi("The surface for channel affinity is not stable yet and may change in the future.")
@Beta
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert back to @BetaApi everywhere

public GrpcCallContext withChannelAffinity(@Nullable Integer affinity) {
return new GrpcCallContext(
channel,
Expand All @@ -214,10 +249,12 @@ public GrpcCallContext withChannelAffinity(@Nullable Integer affinity) {
streamWaitTimeout,
streamIdleTimeout,
affinity,
extraHeaders);
extraHeaders,
retrySettings,
retryableCodes);
}

@BetaApi("The surface for extra headers is not stable yet and may change in the future.")
@Beta
@Override
public GrpcCallContext withExtraHeaders(Map<String, List<String>> extraHeaders) {
Preconditions.checkNotNull(extraHeaders);
Expand All @@ -230,7 +267,47 @@ public GrpcCallContext withExtraHeaders(Map<String, List<String>> extraHeaders)
streamWaitTimeout,
streamIdleTimeout,
channelAffinity,
newExtraHeaders);
newExtraHeaders,
retrySettings,
retryableCodes);
}

@Override
public RetrySettings getRetrySettings() {
return this.retrySettings;
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between the retry settings here, and those in UnaryCallSettings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The RetrySettings in UnaryCallSettings are the default retry settings that are generated as part of the gapic client code generation. These settings are used if the client application does not set any RetrySettings on the CallContext that is used for an invocation of the RPC.
  • The RetrySettings that are set on the CallContext will override the settings in UnaryCallSettings for the invocation(s) where the CallContext is used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, UnaryCallSettings are available at "client-creation", not at run time. As @olavloite mentioned, the UnaryCallSettings supply the default settings for a Callable at creation. The RetrySettings are extracted from the UnaryCallSettings and used throughout the chain independently.

public GrpcCallContext withRetrySettings(RetrySettings retrySettings) {
return new GrpcCallContext(
this.channel,
this.callOptions,
this.timeout,
this.streamWaitTimeout,
this.streamIdleTimeout,
this.channelAffinity,
this.extraHeaders,
retrySettings,
retryableCodes);
}

@Override
public Set<StatusCode.Code> getRetryableCodes() {
return this.retryableCodes;
}

@Override
public GrpcCallContext withRetryableCodes(Set<StatusCode.Code> retryableCodes) {
return new GrpcCallContext(
this.channel,
this.callOptions,
this.timeout,
this.streamWaitTimeout,
this.streamIdleTimeout,
this.channelAffinity,
this.extraHeaders,
this.retrySettings,
retryableCodes);
}

@Override
Expand Down Expand Up @@ -285,6 +362,16 @@ public ApiCallContext merge(ApiCallContext inputCallContext) {
newChannelAffinity = this.channelAffinity;
}

RetrySettings newRetrySettings = grpcCallContext.retrySettings;
if (newRetrySettings == null) {
newRetrySettings = this.retrySettings;
}

Set<StatusCode.Code> newRetryableCodes = grpcCallContext.retryableCodes;
if (newRetryableCodes == null) {
newRetryableCodes = this.retryableCodes;
}

ImmutableMap<String, List<String>> newExtraHeaders =
Headers.mergeHeaders(extraHeaders, grpcCallContext.extraHeaders);

Expand All @@ -305,7 +392,9 @@ public ApiCallContext merge(ApiCallContext inputCallContext) {
newStreamWaitTimeout,
newStreamIdleTimeout,
newChannelAffinity,
newExtraHeaders);
newExtraHeaders,
newRetrySettings,
newRetryableCodes);
}

/** The {@link Channel} set on this context. */
Expand All @@ -323,7 +412,7 @@ public CallOptions getCallOptions() {
*
* @see ApiCallContext#withStreamWaitTimeout(Duration)
*/
@BetaApi("The surface for streaming is not stable yet and may change in the future.")
@Beta
@Nullable
public Duration getStreamWaitTimeout() {
return streamWaitTimeout;
Expand All @@ -334,21 +423,21 @@ public Duration getStreamWaitTimeout() {
*
* @see ApiCallContext#withStreamIdleTimeout(Duration)
*/
@BetaApi("The surface for streaming is not stable yet and may change in the future.")
@Beta
@Nullable
public Duration getStreamIdleTimeout() {
return streamIdleTimeout;
}

/** The channel affinity for this context. */
@BetaApi("The surface for channel affinity is not stable yet and may change in the future.")
@Beta
@Nullable
public Integer getChannelAffinity() {
return channelAffinity;
}

/** The extra header for this context. */
@BetaApi("The surface for extra headers is not stable yet and may change in the future.")
@Beta
@Override
public Map<String, List<String>> getExtraHeaders() {
return this.extraHeaders;
Expand All @@ -363,7 +452,9 @@ public GrpcCallContext withChannel(Channel newChannel) {
this.streamWaitTimeout,
this.streamIdleTimeout,
this.channelAffinity,
this.extraHeaders);
this.extraHeaders,
this.retrySettings,
this.retryableCodes);
}

/** Returns a new instance with the call options set to the given call options. */
Expand All @@ -375,7 +466,9 @@ public GrpcCallContext withCallOptions(CallOptions newCallOptions) {
this.streamWaitTimeout,
this.streamIdleTimeout,
this.channelAffinity,
this.extraHeaders);
this.extraHeaders,
this.retrySettings,
this.retryableCodes);
}

public GrpcCallContext withRequestParamsDynamicHeaderOption(String requestParams) {
Expand Down Expand Up @@ -412,7 +505,9 @@ public int hashCode() {
streamWaitTimeout,
streamIdleTimeout,
channelAffinity,
extraHeaders);
extraHeaders,
retrySettings,
retryableCodes);
}

@Override
Expand All @@ -431,7 +526,9 @@ public boolean equals(Object o) {
&& Objects.equals(streamWaitTimeout, that.streamWaitTimeout)
&& Objects.equals(streamIdleTimeout, that.streamIdleTimeout)
&& Objects.equals(channelAffinity, that.channelAffinity)
&& Objects.equals(extraHeaders, that.extraHeaders);
&& Objects.equals(extraHeaders, that.extraHeaders)
&& Objects.equals(retrySettings, that.retrySettings)
&& Objects.equals(retryableCodes, that.retryableCodes);
}

Metadata getMetadata() {
Expand Down
Expand Up @@ -30,8 +30,12 @@
package com.google.api.gax.grpc;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;

import com.google.api.gax.retrying.RetrySettings;
import com.google.api.gax.rpc.ApiCallContext;
import com.google.api.gax.rpc.StatusCode;
import com.google.api.gax.rpc.testing.FakeCallContext;
import com.google.api.gax.rpc.testing.FakeChannel;
import com.google.api.gax.rpc.testing.FakeTransportChannel;
Expand All @@ -43,9 +47,11 @@
import io.grpc.ManagedChannel;
import io.grpc.Metadata.Key;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -72,9 +78,9 @@ public void testNullToSelfWrongType() {
public void testWithCredentials() {
Credentials credentials = Mockito.mock(Credentials.class);
GrpcCallContext emptyContext = GrpcCallContext.createDefault();
Truth.assertThat(emptyContext.getCallOptions().getCredentials()).isNull();
assertNull(emptyContext.getCallOptions().getCredentials());
GrpcCallContext context = emptyContext.withCredentials(credentials);
Truth.assertThat(context.getCallOptions().getCredentials()).isNotNull();
assertNotNull(context.getCallOptions().getCredentials());
}

@Test
Expand Down Expand Up @@ -123,21 +129,17 @@ public void testWithRequestParamsDynamicHeaderOption() {

@Test
public void testWithTimeout() {
Truth.assertThat(GrpcCallContext.createDefault().withTimeout(null).getTimeout()).isNull();
assertNull(GrpcCallContext.createDefault().withTimeout(null).getTimeout());
}

@Test
public void testWithNegativeTimeout() {
Truth.assertThat(
GrpcCallContext.createDefault().withTimeout(Duration.ofSeconds(-1L)).getTimeout())
.isNull();
assertNull(GrpcCallContext.createDefault().withTimeout(Duration.ofSeconds(-1L)).getTimeout());
}

@Test
public void testWithZeroTimeout() {
Truth.assertThat(
GrpcCallContext.createDefault().withTimeout(Duration.ofSeconds(0L)).getTimeout())
.isNull();
assertNull(GrpcCallContext.createDefault().withTimeout(Duration.ofSeconds(0L)).getTimeout());
}

@Test
Expand Down Expand Up @@ -334,6 +336,24 @@ public void testMergeWithTracer() {
.isSameInstanceAs(defaultTracer);
}

@Test
public void testWithRetrySettings() {
RetrySettings retrySettings = Mockito.mock(RetrySettings.class);
GrpcCallContext emptyContext = GrpcCallContext.createDefault();
assertNull(emptyContext.getRetrySettings());
GrpcCallContext context = emptyContext.withRetrySettings(retrySettings);
assertNotNull(context.getRetrySettings());
}

@Test
public void testWithRetryableCodes() {
Set<StatusCode.Code> codes = Collections.singleton(StatusCode.Code.UNAVAILABLE);
GrpcCallContext emptyContext = GrpcCallContext.createDefault();
assertNull(emptyContext.getRetryableCodes());
GrpcCallContext context = emptyContext.withRetryableCodes(codes);
assertNotNull(context.getRetryableCodes());
}

private static Map<String, List<String>> createTestExtraHeaders(String... keyValues) {
Map<String, List<String>> extraHeaders = new HashMap<>();
for (int i = 0; i < keyValues.length; i += 2) {
Expand Down