Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: handle retry info so client respect the delay server sets #2026

Merged
merged 14 commits into from
Dec 19, 2023
Prev Previous commit
Next Next commit
address comments
  • Loading branch information
mutianf committed Dec 19, 2023
commit 99c43c57b65927457569a7dac723eb11d3cf4072
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.api.gax.grpc.GrpcCallSettings;
import com.google.api.gax.grpc.GrpcRawCallableFactory;
import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider;
import com.google.api.gax.retrying.BasicResultRetryAlgorithm;
import com.google.api.gax.retrying.ExponentialRetryAlgorithm;
import com.google.api.gax.retrying.RetryAlgorithm;
import com.google.api.gax.retrying.RetryingExecutorWithContext;
Expand Down Expand Up @@ -763,18 +764,19 @@ public Map<String, String> extract(MutateRowsRequest mutateRowsRequest) {
ServerStreamingCallable<MutateRowsRequest, MutateRowsResponse> withBigtableTracer =
new BigtableTracerStreamingCallable<>(convertException);

RetryAlgorithm<Void> retryAlgorithm;
ExponentialRetryAlgorithm exponentialRetryAlgorithm =
new ExponentialRetryAlgorithm(
settings.bulkMutateRowsSettings().getRetrySettings(), clientContext.getClock());
BasicResultRetryAlgorithm<Void> resultRetryAlgorithm;
if (settings.getEnableRetryInfo()) {
retryAlgorithm =
new RetryAlgorithm<>(new RetryInfoRetryAlgorithm<>(), exponentialRetryAlgorithm);
resultRetryAlgorithm = new RetryInfoRetryAlgorithm<>();
} else {
retryAlgorithm =
new RetryAlgorithm<>(new ApiResultRetryAlgorithm<>(), exponentialRetryAlgorithm);
resultRetryAlgorithm = new ApiResultRetryAlgorithm<>();
}

RetryAlgorithm<Void> retryAlgorithm =
new RetryAlgorithm<>(
resultRetryAlgorithm,
new ExponentialRetryAlgorithm(
settings.bulkMutateRowsSettings().getRetrySettings(), clientContext.getClock()));

RetryingExecutorWithContext<Void> retryingExecutor =
new ScheduledRetryingExecutor<>(retryAlgorithm, clientContext.getExecutor());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ public boolean getEnableRoutingCookie() {
* Gets if RetryInfo is enabled. If true, client bases retry decision and back off time on server
* returned RetryInfo value. Otherwise, client uses {@link RetrySettings}.
*/
@BetaApi("RetryInfo is not currently stable and may change in the future")
public boolean getEnableRetryInfo() {
mutianf marked this conversation as resolved.
Show resolved Hide resolved
return enableRetryInfo;
}
Expand Down Expand Up @@ -931,15 +932,6 @@ public Builder setEnableRoutingCookie(boolean enableRoutingCookie) {
return this;
}

/**
* Sets if RetryInfo is enabled. If true, client bases retry decision and back off time on
* server returned RetryInfo value. Otherwise, client uses {@link RetrySettings}.
*/
public Builder setEnableRetryInfo(boolean enableRetryInfo) {
this.enableRetryInfo = enableRetryInfo;
return this;
}

/**
* Gets if routing cookie is enabled. If true, client will retry a request with extra metadata
* server sent back.
Expand All @@ -949,10 +941,21 @@ public boolean getEnableRoutingCookie() {
return enableRoutingCookie;
}

/**
* Sets if RetryInfo is enabled. If true, client bases retry decision and back off time on
* server returned RetryInfo value. Otherwise, client uses {@link RetrySettings}.
*/
@BetaApi("RetryInfo is not currently stable and may change in the future")
public Builder setEnableRetryInfo(boolean enableRetryInfo) {
this.enableRetryInfo = enableRetryInfo;
return this;
}

/**
* Gets if RetryInfo is enabled. If true, client bases retry decision and back off time on
* server returned RetryInfo value. Otherwise, client uses {@link RetrySettings}.
*/
@BetaApi("RetryInfo is not currently stable and may change in the future")
public boolean getEnableRetryInfo() {
mutianf marked this conversation as resolved.
Show resolved Hide resolved
return enableRetryInfo;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
@InternalApi
public class ApiException {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ApiExceptions. Also please add a private ctor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

In general the pattern is that the instance class is singular and the helper utility with static methods is plural.
In this case you are adding latter. Eventually when you upstream, you will make it an instance method and it will be in ApiException. But in the transitional state I think plural makes sense


private ApiException() {}

// TODO: this should replace the existing ApiException#isRetryable() method,
// but that cant be done in bigtable, so this lives here for now.
public static boolean isRetryable2(Throwable e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import io.grpc.Metadata;
import io.grpc.Status;
import io.grpc.protobuf.ProtoUtils;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.threeten.bp.Duration;

// TODO move this algorithm to gax
Expand Down Expand Up @@ -57,13 +58,7 @@ public TimedAttemptSettings createNextAttempt(
/** Returns true if previousThrowable is an {@link ApiException} that is retryable. */
@Override
public boolean shouldRetry(Throwable previousThrowable, ResponseT previousResponse) {
if (extractRetryDelay(previousThrowable) != null) {
// First check if server wants us to retry
return true;
}
// Server didn't have retry information, use the local status code config.
return (previousThrowable instanceof ApiException
&& ((ApiException) previousThrowable).isRetryable());
return shouldRetry(null, previousThrowable, previousResponse);
}

/**
Expand All @@ -74,23 +69,27 @@ public boolean shouldRetry(Throwable previousThrowable, ResponseT previousRespon
*/
@Override
public boolean shouldRetry(
RetryingContext context, Throwable previousThrowable, ResponseT previousResponse) {
@Nullable RetryingContext context, Throwable previousThrowable, ResponseT previousResponse) {
if (extractRetryDelay(previousThrowable) != null) {
// First check if server wants us to retry
return true;
}
if (context.getRetryableCodes() != null) {
if (context != null && context.getRetryableCodes() != null) {
// Ignore the isRetryable() value of the throwable if the RetryingContext has a specific list
// of codes that should be retried.
return ((previousThrowable instanceof ApiException)
&& context
.getRetryableCodes()
.contains(((ApiException) previousThrowable).getStatusCode().getCode()));
}
return shouldRetry(previousThrowable, previousResponse);
// Server didn't have retry information and there's no retry context, use the local status
// code config.
return previousThrowable instanceof ApiException
&& ((ApiException) previousThrowable).isRetryable();
}

static Duration extractRetryDelay(Throwable throwable) {
@Nullable
static Duration extractRetryDelay(@Nullable Throwable throwable) {
if (throwable == null) {
return null;
}
Expand Down
Loading
Loading