From 527cac4122a3edd8afac7356a758cdf2f2fd49fa Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Thu, 28 Dec 2023 16:44:17 -0500 Subject: [PATCH 1/8] fix: fix RetryInfo algorithm and tests --- .../data/v2/models/MutateRowsException.java | 22 ++++- .../mutaterows/MutateRowsAttemptCallable.java | 26 ++++-- .../retrying/RetryInfoRetryAlgorithm.java | 11 ++- .../bigtable/data/v2/stub/RetryInfoTest.java | 84 +++++++++++++++++-- 4 files changed, 122 insertions(+), 21 deletions(-) diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java index d1c0eda844..bdd2136b62 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java @@ -17,6 +17,7 @@ import com.google.api.core.InternalApi; import com.google.api.gax.rpc.ApiException; +import com.google.api.gax.rpc.ErrorDetails; import com.google.api.gax.rpc.StatusCode; import com.google.auto.value.AutoValue; import com.google.bigtable.v2.MutateRowsRequest; @@ -56,13 +57,30 @@ public Object getTransportCode() { public MutateRowsException( @Nullable Throwable rpcError, @Nonnull List failedMutations, - boolean retryable) { - super("Some mutations failed to apply", rpcError, LOCAL_STATUS, retryable); + boolean retryable, + @Nullable ErrorDetails errorDetails) { + super( + new Throwable("Some mutations failed to apply", rpcError), + LOCAL_STATUS, + retryable, + errorDetails); Preconditions.checkNotNull(failedMutations); Preconditions.checkArgument(!failedMutations.isEmpty(), "failedMutations can't be empty"); this.failedMutations = failedMutations; } + /** + * This constructor is considered an internal implementation detail and not meant to be used by + * applications. + */ + @InternalApi + public MutateRowsException( + @Nullable Throwable rpcError, + @Nonnull List failedMutations, + boolean retryable) { + this(rpcError, failedMutations, retryable, null); + } + /** * Retrieve all of the failed mutations. This list will contain failures for all of the mutations * that have failed across all of the retry attempts so far. diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java index 269ce79031..3dc78610f5 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java @@ -23,6 +23,7 @@ import com.google.api.gax.rpc.ApiCallContext; import com.google.api.gax.rpc.ApiException; import com.google.api.gax.rpc.ApiExceptionFactory; +import com.google.api.gax.rpc.ErrorDetails; import com.google.api.gax.rpc.StatusCode; import com.google.api.gax.rpc.UnaryCallable; import com.google.bigtable.v2.MutateRowsRequest; @@ -250,7 +251,12 @@ private void handleAttemptError(Throwable rpcError) { currentRequest = builder.build(); originalIndexes = newOriginalIndexes; - throw new MutateRowsException(rpcError, allFailures.build(), entryError.isRetryable()); + ErrorDetails errorDetails = null; + if (rpcError instanceof ApiException) { + errorDetails = ((ApiException) rpcError).getErrorDetails(); + } + throw new MutateRowsException( + rpcError, allFailures.build(), entryError.isRetryable(), errorDetails); } /** @@ -268,6 +274,8 @@ private void handleAttemptSuccess(List responses) { List newOriginalIndexes = Lists.newArrayList(); boolean[] seenIndices = new boolean[currentRequest.getEntriesCount()]; + ErrorDetails errorDetails = null; + for (MutateRowsResponse response : responses) { for (Entry entry : response.getEntriesList()) { seenIndices[Ints.checkedCast(entry.getIndex())] = true; @@ -283,13 +291,15 @@ private void handleAttemptSuccess(List responses) { allFailures.add(failedMutation); - if (!failedMutation.getError().isRetryable()) { + if (!ApiExceptions.isRetryable2(failedMutation.getError()) + && !failedMutation.getError().isRetryable()) { permanentFailures.add(failedMutation); } else { // Schedule the mutation entry for the next RPC by adding it to the request builder and // recording it's original index newOriginalIndexes.add(origIndex); builder.addEntries(lastRequest.getEntries((int) entry.getIndex())); + errorDetails = failedMutation.getError().getErrorDetails(); } } } @@ -319,7 +329,7 @@ private void handleAttemptSuccess(List responses) { if (!allFailures.isEmpty()) { boolean isRetryable = builder.getEntriesCount() > 0; - throw new MutateRowsException(null, allFailures, isRetryable); + throw new MutateRowsException(null, allFailures, isRetryable, errorDetails); } } @@ -338,10 +348,14 @@ private ApiException createEntryError(com.google.rpc.Status protoStatus) { StatusCode gaxStatusCode = GrpcStatusCode.of(grpcStatus.getCode()); + ErrorDetails errorDetails = + ErrorDetails.builder().setRawErrorMessages(protoStatus.getDetailsList()).build(); + return ApiExceptionFactory.createException( grpcStatus.asRuntimeException(), gaxStatusCode, - retryableCodes.contains(gaxStatusCode.getCode())); + retryableCodes.contains(gaxStatusCode.getCode()), + errorDetails); } /** @@ -354,10 +368,10 @@ private static ApiException createSyntheticErrorForRpcFailure(Throwable overallR ApiException requestApiException = (ApiException) overallRequestError; return ApiExceptionFactory.createException( - "Didn't receive a result for this mutation entry", overallRequestError, requestApiException.getStatusCode(), - requestApiException.isRetryable()); + requestApiException.isRetryable(), + requestApiException.getErrorDetails()); } return ApiExceptionFactory.createException( diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java index 71457f7e9a..0d5802e1fe 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java @@ -24,7 +24,6 @@ import com.google.protobuf.util.Durations; import com.google.rpc.RetryInfo; 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; @@ -93,17 +92,17 @@ static Duration extractRetryDelay(@Nullable Throwable throwable) { if (throwable == null) { return null; } - Metadata trailers = Status.trailersFromThrowable(throwable); - if (trailers == null) { + if (!(throwable instanceof ApiException)) { return null; } - RetryInfo retryInfo = trailers.get(RETRY_INFO_KEY); - if (retryInfo == null) { + ApiException exception = (ApiException) throwable; + if (exception.getErrorDetails() == null) { return null; } - if (!retryInfo.hasRetryDelay()) { + if (exception.getErrorDetails().getRetryInfo() == null) { return null; } + RetryInfo retryInfo = exception.getErrorDetails().getRetryInfo(); return Duration.ofMillis(Durations.toMillis(retryInfo.getRetryDelay())); } } diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java index b38e53480c..7c53dc6034 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java @@ -15,13 +15,13 @@ */ package com.google.cloud.bigtable.data.v2.stub; -import static com.google.cloud.bigtable.gaxx.retrying.RetryInfoRetryAlgorithm.RETRY_INFO_KEY; import static com.google.common.truth.Truth.assertThat; import com.google.api.gax.core.NoCredentialsProvider; import com.google.api.gax.grpc.GrpcStatusCode; import com.google.api.gax.grpc.GrpcTransportChannel; import com.google.api.gax.rpc.ApiException; +import com.google.api.gax.rpc.ErrorDetails; import com.google.api.gax.rpc.FixedTransportChannelProvider; import com.google.api.gax.rpc.InternalException; import com.google.api.gax.rpc.UnavailableException; @@ -55,7 +55,9 @@ import com.google.cloud.bigtable.data.v2.models.RowMutation; import com.google.cloud.bigtable.data.v2.models.RowMutationEntry; import com.google.common.base.Stopwatch; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Queues; +import com.google.protobuf.Any; import com.google.rpc.RetryInfo; import io.grpc.Metadata; import io.grpc.Status; @@ -77,6 +79,9 @@ public class RetryInfoTest { @Rule public GrpcServerRule serverRule = new GrpcServerRule(); + private static final Metadata.Key ERROR_DETAILS_KEY = + Metadata.Key.of("grpc-status-details-bin", Metadata.BINARY_BYTE_MARSHALLER); + private FakeBigtableService service; private BigtableDataClient client; private BigtableDataSettings.Builder settings; @@ -167,6 +172,42 @@ public void testMutateRowsNonRetryableErrorWithRetryInfo() { false); } + @Test + public void testMutateRowsPartialFailure() { + service.partial = true; + + verifyRetryInfoIsUsed( + () -> + client.bulkMutateRows( + BulkMutation.create("fake-table") + .add(RowMutationEntry.create("row-key-1").setCell("cf", "q", "v"))), + true); + } + + @Test + public void testMutateRowsPartialFailureNonRetryableError() { + service.partial = true; + + verifyRetryInfoIsUsed( + () -> + client.bulkMutateRows( + BulkMutation.create("fake-table") + .add(RowMutationEntry.create("row-key-1").setCell("cf", "q", "v"))), + false); + } + + // TODO: add this test back + // @Test + public void testMutateRowsPartialFailureCanBeDisabled() { + service.partial = true; + + verifyRetryInfoCanBeDisabled( + () -> + client.bulkMutateRows( + BulkMutation.create("fake-table") + .add(RowMutationEntry.create("row-key-1").setCell("cf", "q", "v")))); + } + @Test public void testMutateRowsDisableRetryInfo() throws IOException { settings.stubSettings().setEnableRetryInfo(false); @@ -366,13 +407,18 @@ private void verifyRetryInfoCanBeDisabled(Runnable runnable) { private void enqueueRetryableExceptionWithDelay(com.google.protobuf.Duration delay) { Metadata trailers = new Metadata(); RetryInfo retryInfo = RetryInfo.newBuilder().setRetryDelay(delay).build(); - trailers.put(RETRY_INFO_KEY, retryInfo); + ErrorDetails errorDetails = + ErrorDetails.builder().setRawErrorMessages(ImmutableList.of(Any.pack(retryInfo))).build(); + byte[] status = + com.google.rpc.Status.newBuilder().addDetails(Any.pack(retryInfo)).build().toByteArray(); + trailers.put(ERROR_DETAILS_KEY, status); ApiException exception = new UnavailableException( new StatusRuntimeException(Status.UNAVAILABLE, trailers), GrpcStatusCode.of(Status.Code.UNAVAILABLE), - true); + true, + errorDetails); service.expectations.add(exception); } @@ -380,13 +426,18 @@ private void enqueueRetryableExceptionWithDelay(com.google.protobuf.Duration del private ApiException enqueueNonRetryableExceptionWithDelay(com.google.protobuf.Duration delay) { Metadata trailers = new Metadata(); RetryInfo retryInfo = RetryInfo.newBuilder().setRetryDelay(delay).build(); - trailers.put(RETRY_INFO_KEY, retryInfo); + ErrorDetails errorDetails = + ErrorDetails.builder().setRawErrorMessages(ImmutableList.of(Any.pack(retryInfo))).build(); + byte[] status = + com.google.rpc.Status.newBuilder().addDetails(Any.pack(retryInfo)).build().toByteArray(); + trailers.put(ERROR_DETAILS_KEY, status); ApiException exception = new InternalException( new StatusRuntimeException(Status.INTERNAL, trailers), GrpcStatusCode.of(Status.Code.INTERNAL), - false); + false, + errorDetails); service.expectations.add(exception); @@ -395,6 +446,7 @@ private ApiException enqueueNonRetryableExceptionWithDelay(com.google.protobuf.D private class FakeBigtableService extends BigtableGrpc.BigtableImplBase { Queue expectations = Queues.newArrayDeque(); + boolean partial = false; @Override public void readRows( @@ -434,8 +486,26 @@ public void mutateRows( responseObserver.onNext(builder.build()); responseObserver.onCompleted(); } else { - Exception expectedRpc = expectations.poll(); - responseObserver.onError(expectedRpc); + if (partial) { + ApiException expectedRpc = (ApiException) expectations.poll(); + MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder(); + builder.addEntries( + 0, + MutateRowsResponse.Entry.newBuilder() + .setStatus( + com.google.rpc.Status.newBuilder() + .setCode(expectedRpc.getStatusCode().getCode().getHttpStatusCode()) + .addDetails(Any.pack(expectedRpc.getErrorDetails().getRetryInfo()))) + .build()); + for (int i = 1; i < request.getEntriesCount(); i++) { + builder.addEntriesBuilder().setIndex(i); + } + responseObserver.onNext(builder.build()); + responseObserver.onCompleted(); + } else { + Exception expectedRpc = expectations.poll(); + responseObserver.onError(expectedRpc); + } } } From d37889743cbb3934d932fe4adde4e11176546f74 Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Tue, 2 Jan 2024 10:53:10 -0500 Subject: [PATCH 2/8] fix MutateRows attempt callable --- .../clirr-ignored-differences.xml | 6 ++++ .../data/v2/stub/EnhancedBigtableStub.java | 3 +- .../mutaterows/MutateRowsAttemptCallable.java | 22 ++++++++---- .../MutateRowsRetryingCallable.java | 8 +++-- .../bigtable/gaxx/retrying/ApiExceptions.java | 34 ------------------- .../bigtable/data/v2/stub/RetryInfoTest.java | 22 +++++++----- .../MutateRowsAttemptCallableTest.java | 33 ++++++++++++++---- 7 files changed, 69 insertions(+), 59 deletions(-) delete mode 100644 google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiExceptions.java diff --git a/google-cloud-bigtable/clirr-ignored-differences.xml b/google-cloud-bigtable/clirr-ignored-differences.xml index 1f0ff9f4a1..17f2086a10 100644 --- a/google-cloud-bigtable/clirr-ignored-differences.xml +++ b/google-cloud-bigtable/clirr-ignored-differences.xml @@ -156,4 +156,10 @@ com/google/cloud/bigtable/gaxx/retrying/ApiResultRetryAlgorithm * + + + 7004 + com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsRetryingCallable + * + diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java index a575aa8607..9245682dc1 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java @@ -784,7 +784,8 @@ public Map extract(MutateRowsRequest mutateRowsRequest) { clientContext.getDefaultCallContext(), withBigtableTracer, retryingExecutor, - settings.bulkMutateRowsSettings().getRetryableCodes()); + settings.bulkMutateRowsSettings().getRetryableCodes(), + retryAlgorithm); } /** diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java index 3dc78610f5..e7c112c615 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java @@ -19,7 +19,9 @@ import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutures; import com.google.api.gax.grpc.GrpcStatusCode; +import com.google.api.gax.retrying.RetryAlgorithm; import com.google.api.gax.retrying.RetryingFuture; +import com.google.api.gax.retrying.TimedAttemptSettings; import com.google.api.gax.rpc.ApiCallContext; import com.google.api.gax.rpc.ApiException; import com.google.api.gax.rpc.ApiExceptionFactory; @@ -32,7 +34,6 @@ import com.google.bigtable.v2.MutateRowsResponse.Entry; import com.google.cloud.bigtable.data.v2.models.MutateRowsException; import com.google.cloud.bigtable.data.v2.models.MutateRowsException.FailedMutation; -import com.google.cloud.bigtable.gaxx.retrying.ApiExceptions; import com.google.cloud.bigtable.gaxx.retrying.NonCancellableFuture; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -112,6 +113,8 @@ public Object getTransportCode() { @Nullable private List originalIndexes; @Nonnull private final Set retryableCodes; @Nullable private final List permanentFailures; + @Nonnull private final RetryAlgorithm retryAlgorithm; + @Nonnull private final TimedAttemptSettings attemptSettings; // Parent controller private RetryingFuture externalFuture; @@ -139,11 +142,14 @@ public List apply(Throwable throwable) { @Nonnull UnaryCallable> innerCallable, @Nonnull MutateRowsRequest originalRequest, @Nonnull ApiCallContext callContext, - @Nonnull Set retryableCodes) { + @Nonnull Set retryableCodes, + @Nonnull RetryAlgorithm retryAlgorithm) { this.innerCallable = Preconditions.checkNotNull(innerCallable, "innerCallable"); this.currentRequest = Preconditions.checkNotNull(originalRequest, "currentRequest"); this.callContext = Preconditions.checkNotNull(callContext, "callContext"); this.retryableCodes = Preconditions.checkNotNull(retryableCodes, "retryableCodes"); + this.retryAlgorithm = retryAlgorithm; + this.attemptSettings = retryAlgorithm.createFirstAttempt(); permanentFailures = Lists.newArrayList(); } @@ -237,8 +243,9 @@ private void handleAttemptError(Throwable rpcError) { FailedMutation failedMutation = FailedMutation.create(origIndex, entryError); allFailures.add(failedMutation); - if (!ApiExceptions.isRetryable2(failedMutation.getError()) - && !failedMutation.getError().isRetryable()) { + TimedAttemptSettings nextAttemptSettings = + retryAlgorithm.createNextAttempt(null, failedMutation.getError(), null, attemptSettings); + if (!retryAlgorithm.shouldRetry(null, failedMutation.getError(), null, nextAttemptSettings)) { permanentFailures.add(failedMutation); } else { // Schedule the mutation entry for the next RPC by adding it to the request builder and @@ -291,8 +298,11 @@ private void handleAttemptSuccess(List responses) { allFailures.add(failedMutation); - if (!ApiExceptions.isRetryable2(failedMutation.getError()) - && !failedMutation.getError().isRetryable()) { + TimedAttemptSettings nextAttemptSettings = + retryAlgorithm.createNextAttempt( + null, failedMutation.getError(), null, attemptSettings); + if (!retryAlgorithm.shouldRetry( + null, failedMutation.getError(), null, nextAttemptSettings)) { permanentFailures.add(failedMutation); } else { // Schedule the mutation entry for the next RPC by adding it to the request builder and diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsRetryingCallable.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsRetryingCallable.java index ff0daf78bb..8ad1db258d 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsRetryingCallable.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsRetryingCallable.java @@ -16,6 +16,7 @@ package com.google.cloud.bigtable.data.v2.stub.mutaterows; import com.google.api.core.InternalApi; +import com.google.api.gax.retrying.RetryAlgorithm; import com.google.api.gax.retrying.RetryingExecutorWithContext; import com.google.api.gax.retrying.RetryingFuture; import com.google.api.gax.rpc.ApiCallContext; @@ -44,23 +45,26 @@ public class MutateRowsRetryingCallable extends UnaryCallable callable; private final RetryingExecutorWithContext executor; private final ImmutableSet retryCodes; + private final RetryAlgorithm retryAlgorithm; public MutateRowsRetryingCallable( @Nonnull ApiCallContext callContextPrototype, @Nonnull ServerStreamingCallable callable, @Nonnull RetryingExecutorWithContext executor, - @Nonnull Set retryCodes) { + @Nonnull Set retryCodes, + @Nonnull RetryAlgorithm retryAlgorithm) { this.callContextPrototype = Preconditions.checkNotNull(callContextPrototype); this.callable = Preconditions.checkNotNull(callable); this.executor = Preconditions.checkNotNull(executor); this.retryCodes = ImmutableSet.copyOf(retryCodes); + this.retryAlgorithm = retryAlgorithm; } @Override public RetryingFuture futureCall(MutateRowsRequest request, ApiCallContext inputContext) { ApiCallContext context = callContextPrototype.nullToSelf(inputContext); MutateRowsAttemptCallable retryCallable = - new MutateRowsAttemptCallable(callable.all(), request, context, retryCodes); + new MutateRowsAttemptCallable(callable.all(), request, context, retryCodes, retryAlgorithm); RetryingFuture retryingFuture = executor.createFuture(retryCallable, context); retryCallable.setExternalFuture(retryingFuture); diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiExceptions.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiExceptions.java deleted file mode 100644 index 4e794fa41a..0000000000 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/ApiExceptions.java +++ /dev/null @@ -1,34 +0,0 @@ -/* - * Copyright 2023 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package com.google.cloud.bigtable.gaxx.retrying; - -import com.google.api.core.InternalApi; - -// TODO: move this to gax later -@InternalApi -public class ApiExceptions { - - private ApiExceptions() {} - - // 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) { - if (RetryInfoRetryAlgorithm.extractRetryDelay(e) != null) { - return true; - } - return false; - } -} diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java index 7c53dc6034..07bc9c0afa 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java @@ -196,16 +196,18 @@ public void testMutateRowsPartialFailureNonRetryableError() { false); } - // TODO: add this test back - // @Test - public void testMutateRowsPartialFailureCanBeDisabled() { + @Test + public void testMutateRowsPartialFailureDisableRetryInfo() throws IOException { service.partial = true; + settings.stubSettings().setEnableRetryInfo(false); - verifyRetryInfoCanBeDisabled( - () -> - client.bulkMutateRows( - BulkMutation.create("fake-table") - .add(RowMutationEntry.create("row-key-1").setCell("cf", "q", "v")))); + try (BigtableDataClient newClient = BigtableDataClient.create(settings.build())) { + verifyRetryInfoCanBeDisabled( + () -> + newClient.bulkMutateRows( + BulkMutation.create("fake-table") + .add(RowMutationEntry.create("row-key-1").setCell("cf", "q", "v")))); + } } @Test @@ -494,7 +496,9 @@ public void mutateRows( MutateRowsResponse.Entry.newBuilder() .setStatus( com.google.rpc.Status.newBuilder() - .setCode(expectedRpc.getStatusCode().getCode().getHttpStatusCode()) + .setCode( + Status.Code.valueOf(expectedRpc.getStatusCode().getCode().name()) + .value()) .addDetails(Any.pack(expectedRpc.getErrorDetails().getRetryInfo()))) .build()); for (int i = 1; i < request.getEntriesCount(); i++) { diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallableTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallableTest.java index 358ff01cde..e5d12ccaeb 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallableTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallableTest.java @@ -16,16 +16,19 @@ package com.google.cloud.bigtable.data.v2.stub.mutaterows; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.any; import com.google.api.core.AbstractApiFuture; import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutures; import com.google.api.gax.grpc.GrpcCallContext; import com.google.api.gax.grpc.GrpcStatusCode; +import com.google.api.gax.retrying.RetryAlgorithm; import com.google.api.gax.retrying.RetrySettings; import com.google.api.gax.retrying.RetryingFuture; import com.google.api.gax.retrying.TimedAttemptSettings; import com.google.api.gax.rpc.ApiCallContext; +import com.google.api.gax.rpc.ApiException; import com.google.api.gax.rpc.StatusCode.Code; import com.google.api.gax.rpc.UnaryCallable; import com.google.api.gax.rpc.UnavailableException; @@ -47,6 +50,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.Mockito; import org.threeten.bp.Duration; @RunWith(JUnit4.class) @@ -64,6 +68,8 @@ public class MutateRowsAttemptCallableTest { private Set retryCodes; private ApiCallContext callContext; private MockRetryingFuture parentFuture; + private final RetryAlgorithm mockRetryAlgorithm = + Mockito.mock(RetryAlgorithm.class); @Before public void setUp() { @@ -71,6 +77,12 @@ public void setUp() { retryCodes = ImmutableSet.of(Code.DEADLINE_EXCEEDED, Code.UNAVAILABLE); callContext = GrpcCallContext.createDefault(); parentFuture = new MockRetryingFuture(); + Mockito.when(mockRetryAlgorithm.shouldRetry(any(), any(), any(), any())) + .thenAnswer( + input -> { + Throwable throwable = input.getArgument(1); + return ((ApiException) throwable).isRetryable(); + }); } @Test @@ -84,7 +96,8 @@ public void singleEntrySuccessTest() throws Exception { .build()); MutateRowsAttemptCallable attemptCallable = - new MutateRowsAttemptCallable(innerCallable, request, callContext, retryCodes); + new MutateRowsAttemptCallable( + innerCallable, request, callContext, retryCodes, mockRetryAlgorithm); attemptCallable.setExternalFuture(parentFuture); attemptCallable.call(); @@ -107,7 +120,8 @@ public void missingEntry() { .build()); MutateRowsAttemptCallable attemptCallable = - new MutateRowsAttemptCallable(innerCallable, request, callContext, retryCodes); + new MutateRowsAttemptCallable( + innerCallable, request, callContext, retryCodes, mockRetryAlgorithm); attemptCallable.setExternalFuture(parentFuture); attemptCallable.call(); @@ -140,7 +154,8 @@ public void testNoRpcTimeout() { .build()); MutateRowsAttemptCallable attemptCallable = - new MutateRowsAttemptCallable(innerCallable, request, callContext, retryCodes); + new MutateRowsAttemptCallable( + innerCallable, request, callContext, retryCodes, mockRetryAlgorithm); attemptCallable.setExternalFuture(parentFuture); attemptCallable.call(); @@ -172,7 +187,8 @@ public void mixedTest() { .build()); MutateRowsAttemptCallable attemptCallable = - new MutateRowsAttemptCallable(innerCallable, request, callContext, retryCodes); + new MutateRowsAttemptCallable( + innerCallable, request, callContext, retryCodes, mockRetryAlgorithm); attemptCallable.setExternalFuture(parentFuture); // Make the only call @@ -230,7 +246,8 @@ public void nextAttemptTest() { .build()); MutateRowsAttemptCallable attemptCallable = - new MutateRowsAttemptCallable(innerCallable, request, callContext, retryCodes); + new MutateRowsAttemptCallable( + innerCallable, request, callContext, retryCodes, mockRetryAlgorithm); attemptCallable.setExternalFuture(parentFuture); // Make the first call @@ -295,7 +312,8 @@ public ApiFuture> futureCall( // Make the call MutateRowsAttemptCallable attemptCallable = - new MutateRowsAttemptCallable(innerCallable, request, callContext, retryCodes); + new MutateRowsAttemptCallable( + innerCallable, request, callContext, retryCodes, mockRetryAlgorithm); attemptCallable.setExternalFuture(parentFuture); attemptCallable.call(); @@ -347,7 +365,8 @@ public ApiFuture> futureCall( // Make the call MutateRowsAttemptCallable attemptCallable = - new MutateRowsAttemptCallable(innerCallable, request, callContext, retryCodes); + new MutateRowsAttemptCallable( + innerCallable, request, callContext, retryCodes, mockRetryAlgorithm); attemptCallable.setExternalFuture(parentFuture); attemptCallable.call(); From 116d53e67c909d9480bb5744c8b3878e61a8efdb Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Tue, 2 Jan 2024 16:08:37 -0500 Subject: [PATCH 3/8] add overall attempt --- .../cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java | 1 + 1 file changed, 1 insertion(+) diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java index 0d5802e1fe..b3cde2a887 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java @@ -49,6 +49,7 @@ public TimedAttemptSettings createNextAttempt( .toBuilder() .setRandomizedRetryDelay(retryDelay) .setAttemptCount(prevSettings.getAttemptCount() + 1) + .setOverallAttemptCount(prevSettings.getAttemptCount() + 1) .build(); } return null; From 05b015a6e150a0dcdada9e1a4980ad6aacb7ba33 Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Fri, 5 Jan 2024 12:10:40 -0500 Subject: [PATCH 4/8] small update --- .../data/v2/stub/mutaterows/MutateRowsAttemptCallable.java | 2 +- .../bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java index e7c112c615..131e73c82e 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java @@ -263,7 +263,7 @@ private void handleAttemptError(Throwable rpcError) { errorDetails = ((ApiException) rpcError).getErrorDetails(); } throw new MutateRowsException( - rpcError, allFailures.build(), entryError.isRetryable(), errorDetails); + rpcError, allFailures.build(), builder.getEntriesCount() > 0, errorDetails); } /** diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java index b3cde2a887..085b48bbb5 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/retrying/RetryInfoRetryAlgorithm.java @@ -20,11 +20,8 @@ import com.google.api.gax.retrying.RetryingContext; import com.google.api.gax.retrying.TimedAttemptSettings; import com.google.api.gax.rpc.ApiException; -import com.google.common.annotations.VisibleForTesting; import com.google.protobuf.util.Durations; import com.google.rpc.RetryInfo; -import io.grpc.Metadata; -import io.grpc.protobuf.ProtoUtils; import org.checkerframework.checker.nullness.qual.Nullable; import org.threeten.bp.Duration; @@ -36,10 +33,6 @@ @InternalApi public class RetryInfoRetryAlgorithm extends BasicResultRetryAlgorithm { - @VisibleForTesting - public static final Metadata.Key RETRY_INFO_KEY = - ProtoUtils.keyForProto(RetryInfo.getDefaultInstance()); - @Override public TimedAttemptSettings createNextAttempt( Throwable prevThrowable, ResponseT prevResponse, TimedAttemptSettings prevSettings) { From dccf29dfd2a204edd10a1a61f6383700437fd68b Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Fri, 5 Jan 2024 13:58:56 -0500 Subject: [PATCH 5/8] remove entry level handling of RetryInfo --- .../clirr-ignored-differences.xml | 14 ++++- .../data/v2/models/MutateRowsException.java | 26 ++++---- .../mutaterows/MutateRowsAttemptCallable.java | 14 +---- .../bigtable/data/v2/stub/RetryInfoTest.java | 63 +------------------ .../MutateRowsBatchingDescriptorTest.java | 2 +- 5 files changed, 32 insertions(+), 87 deletions(-) diff --git a/google-cloud-bigtable/clirr-ignored-differences.xml b/google-cloud-bigtable/clirr-ignored-differences.xml index 17f2086a10..60b9dca093 100644 --- a/google-cloud-bigtable/clirr-ignored-differences.xml +++ b/google-cloud-bigtable/clirr-ignored-differences.xml @@ -159,7 +159,19 @@ 7004 - com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsRetryingCallable + com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsRetryingCallable + * + + + + 7004 + com/google/cloud/bigtable/data/v2/models/MutateRowsException + * + + + + 7009 + com/google/cloud/bigtable/data/v2/models/MutateRowsException * diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java index bdd2136b62..835b550dd6 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java @@ -49,12 +49,24 @@ public Object getTransportCode() { private final List failedMutations; + public static MutateRowsException create( + @Nullable Throwable rpcError, + @Nonnull List failedMutations, + boolean retryable) { + ErrorDetails errorDetails = null; + if (rpcError instanceof ApiException) { + errorDetails = ((ApiException) rpcError).getErrorDetails(); + } + + return new MutateRowsException(rpcError, failedMutations, retryable, errorDetails); + } + /** * This constructor is considered an internal implementation detail and not meant to be used by * applications. */ @InternalApi - public MutateRowsException( + private MutateRowsException( @Nullable Throwable rpcError, @Nonnull List failedMutations, boolean retryable, @@ -69,18 +81,6 @@ public MutateRowsException( this.failedMutations = failedMutations; } - /** - * This constructor is considered an internal implementation detail and not meant to be used by - * applications. - */ - @InternalApi - public MutateRowsException( - @Nullable Throwable rpcError, - @Nonnull List failedMutations, - boolean retryable) { - this(rpcError, failedMutations, retryable, null); - } - /** * Retrieve all of the failed mutations. This list will contain failures for all of the mutations * that have failed across all of the retry attempts so far. diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java index 131e73c82e..2bdaad0b3c 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java @@ -258,12 +258,7 @@ private void handleAttemptError(Throwable rpcError) { currentRequest = builder.build(); originalIndexes = newOriginalIndexes; - ErrorDetails errorDetails = null; - if (rpcError instanceof ApiException) { - errorDetails = ((ApiException) rpcError).getErrorDetails(); - } - throw new MutateRowsException( - rpcError, allFailures.build(), builder.getEntriesCount() > 0, errorDetails); + throw MutateRowsException.create(rpcError, allFailures.build(), builder.getEntriesCount() > 0); } /** @@ -271,7 +266,7 @@ private void handleAttemptError(Throwable rpcError) { * transient failures are found, their corresponding mutations are scheduled for the next RPC. The * caller is notified of both new found errors and pre-existing permanent errors in the thrown * {@link MutateRowsException}. If no errors exist, then the attempt future is successfully - * completed. + * completed. We don't currently handle RetryInfo on entry level failures. */ private void handleAttemptSuccess(List responses) { List allFailures = Lists.newArrayList(permanentFailures); @@ -281,8 +276,6 @@ private void handleAttemptSuccess(List responses) { List newOriginalIndexes = Lists.newArrayList(); boolean[] seenIndices = new boolean[currentRequest.getEntriesCount()]; - ErrorDetails errorDetails = null; - for (MutateRowsResponse response : responses) { for (Entry entry : response.getEntriesList()) { seenIndices[Ints.checkedCast(entry.getIndex())] = true; @@ -309,7 +302,6 @@ private void handleAttemptSuccess(List responses) { // recording it's original index newOriginalIndexes.add(origIndex); builder.addEntries(lastRequest.getEntries((int) entry.getIndex())); - errorDetails = failedMutation.getError().getErrorDetails(); } } } @@ -339,7 +331,7 @@ private void handleAttemptSuccess(List responses) { if (!allFailures.isEmpty()) { boolean isRetryable = builder.getEntriesCount() > 0; - throw new MutateRowsException(null, allFailures, isRetryable, errorDetails); + throw MutateRowsException.create(null, allFailures, isRetryable); } } diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java index 07bc9c0afa..6c3f2eab9d 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java @@ -172,44 +172,6 @@ public void testMutateRowsNonRetryableErrorWithRetryInfo() { false); } - @Test - public void testMutateRowsPartialFailure() { - service.partial = true; - - verifyRetryInfoIsUsed( - () -> - client.bulkMutateRows( - BulkMutation.create("fake-table") - .add(RowMutationEntry.create("row-key-1").setCell("cf", "q", "v"))), - true); - } - - @Test - public void testMutateRowsPartialFailureNonRetryableError() { - service.partial = true; - - verifyRetryInfoIsUsed( - () -> - client.bulkMutateRows( - BulkMutation.create("fake-table") - .add(RowMutationEntry.create("row-key-1").setCell("cf", "q", "v"))), - false); - } - - @Test - public void testMutateRowsPartialFailureDisableRetryInfo() throws IOException { - service.partial = true; - settings.stubSettings().setEnableRetryInfo(false); - - try (BigtableDataClient newClient = BigtableDataClient.create(settings.build())) { - verifyRetryInfoCanBeDisabled( - () -> - newClient.bulkMutateRows( - BulkMutation.create("fake-table") - .add(RowMutationEntry.create("row-key-1").setCell("cf", "q", "v")))); - } - } - @Test public void testMutateRowsDisableRetryInfo() throws IOException { settings.stubSettings().setEnableRetryInfo(false); @@ -448,7 +410,6 @@ private ApiException enqueueNonRetryableExceptionWithDelay(com.google.protobuf.D private class FakeBigtableService extends BigtableGrpc.BigtableImplBase { Queue expectations = Queues.newArrayDeque(); - boolean partial = false; @Override public void readRows( @@ -488,28 +449,8 @@ public void mutateRows( responseObserver.onNext(builder.build()); responseObserver.onCompleted(); } else { - if (partial) { - ApiException expectedRpc = (ApiException) expectations.poll(); - MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder(); - builder.addEntries( - 0, - MutateRowsResponse.Entry.newBuilder() - .setStatus( - com.google.rpc.Status.newBuilder() - .setCode( - Status.Code.valueOf(expectedRpc.getStatusCode().getCode().name()) - .value()) - .addDetails(Any.pack(expectedRpc.getErrorDetails().getRetryInfo()))) - .build()); - for (int i = 1; i < request.getEntriesCount(); i++) { - builder.addEntriesBuilder().setIndex(i); - } - responseObserver.onNext(builder.build()); - responseObserver.onCompleted(); - } else { - Exception expectedRpc = expectations.poll(); - responseObserver.onError(expectedRpc); - } + Exception expectedRpc = expectations.poll(); + responseObserver.onError(expectedRpc); } } diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsBatchingDescriptorTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsBatchingDescriptorTest.java index 81d5c67396..237444ba84 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsBatchingDescriptorTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsBatchingDescriptorTest.java @@ -138,7 +138,7 @@ public void splitExceptionWithFailedMutationsTest() { // Threw an exception at 1st and 3rd entry MutateRowsException serverError = - new MutateRowsException( + MutateRowsException.create( null, ImmutableList.of( MutateRowsException.FailedMutation.create( From de12fd396a7cfefc162c0b5a2b1e86b38f43e0f6 Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Fri, 5 Jan 2024 14:07:29 -0500 Subject: [PATCH 6/8] clean up --- .../bigtable/data/v2/models/MutateRowsException.java | 10 +++++----- .../v2/stub/mutaterows/MutateRowsAttemptCallable.java | 7 +------ 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java index 835b550dd6..9042c3ebfd 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java @@ -49,6 +49,11 @@ public Object getTransportCode() { private final List failedMutations; + /** + * This constructor is considered an internal implementation detail and not meant to be used by + * applications. + */ + @InternalApi public static MutateRowsException create( @Nullable Throwable rpcError, @Nonnull List failedMutations, @@ -61,11 +66,6 @@ public static MutateRowsException create( return new MutateRowsException(rpcError, failedMutations, retryable, errorDetails); } - /** - * This constructor is considered an internal implementation detail and not meant to be used by - * applications. - */ - @InternalApi private MutateRowsException( @Nullable Throwable rpcError, @Nonnull List failedMutations, diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java index 2bdaad0b3c..d69bb9a6b6 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java @@ -25,7 +25,6 @@ import com.google.api.gax.rpc.ApiCallContext; import com.google.api.gax.rpc.ApiException; import com.google.api.gax.rpc.ApiExceptionFactory; -import com.google.api.gax.rpc.ErrorDetails; import com.google.api.gax.rpc.StatusCode; import com.google.api.gax.rpc.UnaryCallable; import com.google.bigtable.v2.MutateRowsRequest; @@ -350,14 +349,10 @@ private ApiException createEntryError(com.google.rpc.Status protoStatus) { StatusCode gaxStatusCode = GrpcStatusCode.of(grpcStatus.getCode()); - ErrorDetails errorDetails = - ErrorDetails.builder().setRawErrorMessages(protoStatus.getDetailsList()).build(); - return ApiExceptionFactory.createException( grpcStatus.asRuntimeException(), gaxStatusCode, - retryableCodes.contains(gaxStatusCode.getCode()), - errorDetails); + retryableCodes.contains(gaxStatusCode.getCode())); } /** From 81a09c364075ccd191e091695ed959241a5f74c2 Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Fri, 5 Jan 2024 16:56:05 -0500 Subject: [PATCH 7/8] fix bug --- .../data/v2/models/MutateRowsException.java | 13 ++++++++----- .../mutaterows/MutateRowsAttemptCallable.java | 16 +++++++--------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java index 9042c3ebfd..4ae0606ab9 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/MutateRowsException.java @@ -71,16 +71,19 @@ private MutateRowsException( @Nonnull List failedMutations, boolean retryable, @Nullable ErrorDetails errorDetails) { - super( - new Throwable("Some mutations failed to apply", rpcError), - LOCAL_STATUS, - retryable, - errorDetails); + super(rpcError, LOCAL_STATUS, retryable, errorDetails); Preconditions.checkNotNull(failedMutations); Preconditions.checkArgument(!failedMutations.isEmpty(), "failedMutations can't be empty"); this.failedMutations = failedMutations; } + // TODO: remove this after we add a ctor in gax to pass in a Throwable, a message and error + // details. + @Override + public String getMessage() { + return "Some mutations failed to apply"; + } + /** * Retrieve all of the failed mutations. This list will contain failures for all of the mutations * that have failed across all of the retry attempts so far. diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java index d69bb9a6b6..29d405a4cb 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java @@ -112,8 +112,8 @@ public Object getTransportCode() { @Nullable private List originalIndexes; @Nonnull private final Set retryableCodes; @Nullable private final List permanentFailures; - @Nonnull private final RetryAlgorithm retryAlgorithm; - @Nonnull private final TimedAttemptSettings attemptSettings; + @Nonnull private final RetryAlgorithm retryAlgorithm; + @Nonnull private TimedAttemptSettings attemptSettings; // Parent controller private RetryingFuture externalFuture; @@ -236,14 +236,15 @@ private void handleAttemptError(Throwable rpcError) { Builder builder = lastRequest.toBuilder().clearEntries(); List newOriginalIndexes = Lists.newArrayList(); + TimedAttemptSettings nextAttemptSettings = + retryAlgorithm.createNextAttempt(null, entryError, null, attemptSettings); + for (int i = 0; i < currentRequest.getEntriesCount(); i++) { int origIndex = getOriginalIndex(i); FailedMutation failedMutation = FailedMutation.create(origIndex, entryError); allFailures.add(failedMutation); - TimedAttemptSettings nextAttemptSettings = - retryAlgorithm.createNextAttempt(null, failedMutation.getError(), null, attemptSettings); if (!retryAlgorithm.shouldRetry(null, failedMutation.getError(), null, nextAttemptSettings)) { permanentFailures.add(failedMutation); } else { @@ -256,6 +257,7 @@ private void handleAttemptError(Throwable rpcError) { currentRequest = builder.build(); originalIndexes = newOriginalIndexes; + attemptSettings = nextAttemptSettings; throw MutateRowsException.create(rpcError, allFailures.build(), builder.getEntriesCount() > 0); } @@ -290,11 +292,7 @@ private void handleAttemptSuccess(List responses) { allFailures.add(failedMutation); - TimedAttemptSettings nextAttemptSettings = - retryAlgorithm.createNextAttempt( - null, failedMutation.getError(), null, attemptSettings); - if (!retryAlgorithm.shouldRetry( - null, failedMutation.getError(), null, nextAttemptSettings)) { + if (!failedMutation.getError().isRetryable()) { permanentFailures.add(failedMutation); } else { // Schedule the mutation entry for the next RPC by adding it to the request builder and From d6bf5282f7cc6de80870845bade29300a126267b Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Mon, 8 Jan 2024 10:35:09 -0500 Subject: [PATCH 8/8] update --- .../data/v2/stub/mutaterows/MutateRowsAttemptCallable.java | 6 ++---- .../google/cloud/bigtable/data/v2/stub/RetryInfoTest.java | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java index 29d405a4cb..155ea43211 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java @@ -236,8 +236,7 @@ private void handleAttemptError(Throwable rpcError) { Builder builder = lastRequest.toBuilder().clearEntries(); List newOriginalIndexes = Lists.newArrayList(); - TimedAttemptSettings nextAttemptSettings = - retryAlgorithm.createNextAttempt(null, entryError, null, attemptSettings); + attemptSettings = retryAlgorithm.createNextAttempt(null, entryError, null, attemptSettings); for (int i = 0; i < currentRequest.getEntriesCount(); i++) { int origIndex = getOriginalIndex(i); @@ -245,7 +244,7 @@ private void handleAttemptError(Throwable rpcError) { FailedMutation failedMutation = FailedMutation.create(origIndex, entryError); allFailures.add(failedMutation); - if (!retryAlgorithm.shouldRetry(null, failedMutation.getError(), null, nextAttemptSettings)) { + if (!retryAlgorithm.shouldRetry(null, failedMutation.getError(), null, attemptSettings)) { permanentFailures.add(failedMutation); } else { // Schedule the mutation entry for the next RPC by adding it to the request builder and @@ -257,7 +256,6 @@ private void handleAttemptError(Throwable rpcError) { currentRequest = builder.build(); originalIndexes = newOriginalIndexes; - attemptSettings = nextAttemptSettings; throw MutateRowsException.create(rpcError, allFailures.build(), builder.getEntriesCount() > 0); } diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java index 6c3f2eab9d..fef901ac2b 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/RetryInfoTest.java @@ -88,7 +88,7 @@ public class RetryInfoTest { private AtomicInteger attemptCounter = new AtomicInteger(); private com.google.protobuf.Duration delay = - com.google.protobuf.Duration.newBuilder().setSeconds(1).setNanos(0).build(); + com.google.protobuf.Duration.newBuilder().setSeconds(2).setNanos(0).build(); @Before public void setUp() throws IOException {