From 42a63b046312fde7f1a16b56722fce12bd1f8450 Mon Sep 17 00:00:00 2001 From: Igor Berntein Date: Mon, 11 Sep 2023 18:17:15 -0400 Subject: [PATCH 1/5] fix: add a sanity check that all bulk mutation entries are accounted for Add a fail safe that marks missing entries in a response as permanent errors. Previously the client assumed that all entries were present and only looked for errors Change-Id: Ie3f294fd6bb19ec17662b58bfe9c75a3eed81097 --- .../mutaterows/MutateRowsAttemptCallable.java | 24 ++++++++++++++ .../MutateRowsAttemptCallableTest.java | 33 +++++++++++++++++++ 2 files changed, 57 insertions(+) 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 36c2930bda..b049219a95 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 @@ -35,6 +35,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; +import com.google.common.primitives.Ints; import com.google.common.util.concurrent.MoreExecutors; import com.google.rpc.Code; import java.util.List; @@ -263,9 +264,12 @@ private void handleAttemptSuccess(List responses) { Builder builder = lastRequest.toBuilder().clearEntries(); List newOriginalIndexes = Lists.newArrayList(); + boolean[] seenIndices = new boolean[currentRequest.getEntriesCount()]; for (MutateRowsResponse response : responses) { for (Entry entry : response.getEntriesList()) { + seenIndices[Ints.checkedCast(entry.getIndex())] = true; + if (entry.getStatus().getCode() == Code.OK_VALUE) { continue; } @@ -288,6 +292,26 @@ private void handleAttemptSuccess(List responses) { } } + // Handle missing mutations + for (int i = 0; i < seenIndices.length; i++) { + if (seenIndices[i]) { + continue; + } + + int origIndex = getOriginalIndex(i); + FailedMutation failedMutation = + FailedMutation.create( + origIndex, + ApiExceptionFactory.createException( + "Missing entry response for entry " + origIndex, + null, + GrpcStatusCode.of(io.grpc.Status.Code.INTERNAL), + false)); + + allFailures.add(failedMutation); + permanentFailures.add(failedMutation); + } + currentRequest = builder.build(); originalIndexes = newOriginalIndexes; 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 2df2aaf2b4..358ff01cde 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 @@ -41,6 +41,8 @@ import java.util.List; import java.util.Set; import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -92,6 +94,37 @@ public void singleEntrySuccessTest() throws Exception { assertThat(innerCallable.lastRequest).isEqualTo(request); } + @Test + public void missingEntry() { + MutateRowsRequest request = + MutateRowsRequest.newBuilder() + .addEntries(Entry.getDefaultInstance()) + .addEntries(Entry.getDefaultInstance()) + .build(); + innerCallable.response.add( + MutateRowsResponse.newBuilder() + .addEntries(MutateRowsResponse.Entry.newBuilder().setIndex(0)) + .build()); + + MutateRowsAttemptCallable attemptCallable = + new MutateRowsAttemptCallable(innerCallable, request, callContext, retryCodes); + attemptCallable.setExternalFuture(parentFuture); + attemptCallable.call(); + + ExecutionException executionException = + Assert.assertThrows(ExecutionException.class, () -> parentFuture.attemptFuture.get()); + assertThat(executionException).hasCauseThat().isInstanceOf(MutateRowsException.class); + MutateRowsException e = (MutateRowsException) executionException.getCause(); + + assertThat(e).hasMessageThat().contains("Some mutations failed to apply"); + assertThat(e.getFailedMutations()).hasSize(1); + FailedMutation failedMutation = e.getFailedMutations().get(0); + assertThat(failedMutation.getIndex()).isEqualTo(1); + assertThat(failedMutation.getError()) + .hasMessageThat() + .contains("Missing entry response for entry 1"); + } + @Test public void testNoRpcTimeout() { parentFuture.timedAttemptSettings = From 35adef3bc3414b429a6d4a09e5e8240a646c4610 Mon Sep 17 00:00:00 2001 From: Igor Berntein Date: Mon, 11 Sep 2023 23:23:20 -0400 Subject: [PATCH 2/5] fix tests Change-Id: I7619ef3ca3921ecc92efd65dc16c86548d51af3f --- .../data/v2/stub/metrics/BigtableTracerCallableTest.java | 6 +++++- .../data/v2/stub/metrics/BuiltinMetricsTracerTest.java | 6 +++++- .../bigtable/data/v2/stub/metrics/MetricsTracerTest.java | 7 ++++++- .../data/v2/stub/metrics/StatsHeadersCallableTest.java | 6 +++++- .../data/v2/stub/mutaterows/MutateRowsRetryTest.java | 6 +++++- 5 files changed, 26 insertions(+), 5 deletions(-) diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerCallableTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerCallableTest.java index e783352bf0..d8e3402b84 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerCallableTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerCallableTest.java @@ -403,7 +403,11 @@ public void mutateRow(MutateRowRequest request, StreamObserver observer) { - observer.onNext(MutateRowsResponse.getDefaultInstance()); + MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder(); + for (int i = 0; i < request.getEntriesCount(); i++) { + builder.addEntries(MutateRowsResponse.Entry.newBuilder().setIndex(i)); + } + observer.onNext(builder.build()); observer.onCompleted(); } diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java index c7a47942f2..c2be1ea0ff 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java @@ -648,7 +648,11 @@ public void mutateRows( Thread.sleep(SERVER_LATENCY); } catch (InterruptedException e) { } - responseObserver.onNext(MutateRowsResponse.getDefaultInstance()); + MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder(); + for (int i = 0; i < request.getEntriesCount(); i++) { + builder.addEntriesBuilder().setIndex(i); + } + responseObserver.onNext(builder.build()); responseObserver.onCompleted(); } diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java index da3dd0770a..da989b65dc 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java @@ -422,10 +422,15 @@ public void testBatchMutateRowsThrottledTime() throws Exception { new Answer() { @Override public Object answer(InvocationOnMock invocation) { + MutateRowsRequest request = (MutateRowsRequest) invocation.getArguments()[0]; @SuppressWarnings("unchecked") StreamObserver observer = (StreamObserver) invocation.getArguments()[1]; - observer.onNext(MutateRowsResponse.getDefaultInstance()); + MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder(); + for (int i = 0; i < request.getEntriesCount(); i++) { + builder.addEntriesBuilder().setIndex(i); + } + observer.onNext(builder.build()); observer.onCompleted(); return null; } diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/StatsHeadersCallableTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/StatsHeadersCallableTest.java index 538d4fc246..88a874b8c9 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/StatsHeadersCallableTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/StatsHeadersCallableTest.java @@ -223,7 +223,11 @@ public void mutateRows(MutateRowsRequest request, StreamObserver responseObserver) { attemptCounter.incrementAndGet(); if (expectations.isEmpty()) { - responseObserver.onNext(MutateRowsResponse.getDefaultInstance()); + MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder(); + for (int i = 0; i < request.getEntriesCount(); i++) { + builder.addEntriesBuilder().setIndex(i); + } + responseObserver.onNext(builder.build()); responseObserver.onCompleted(); } else { Exception expectedRpc = expectations.poll(); From 3d48bf5e36de06d16b98a058cf3698b0f160f228 Mon Sep 17 00:00:00 2001 From: Igor Berntein Date: Tue, 12 Sep 2023 16:15:00 -0400 Subject: [PATCH 3/5] codestyle Change-Id: I3d4a5780bd3f7572c9889c10e60b908b1e05b359 --- .../mutaterows/MutateRowsAttemptCallable.java | 39 +++++++++---------- 1 file changed, 19 insertions(+), 20 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 b049219a95..4cb2b2a750 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 @@ -294,30 +294,29 @@ private void handleAttemptSuccess(List responses) { // Handle missing mutations for (int i = 0; i < seenIndices.length; i++) { - if (seenIndices[i]) { - continue; - } + if (!seenIndices[i]) { - int origIndex = getOriginalIndex(i); - FailedMutation failedMutation = - FailedMutation.create( - origIndex, - ApiExceptionFactory.createException( - "Missing entry response for entry " + origIndex, - null, - GrpcStatusCode.of(io.grpc.Status.Code.INTERNAL), - false)); + int origIndex = getOriginalIndex(i); + FailedMutation failedMutation = + FailedMutation.create( + origIndex, + ApiExceptionFactory.createException( + "Missing entry response for entry " + origIndex, + null, + GrpcStatusCode.of(io.grpc.Status.Code.INTERNAL), + false)); - allFailures.add(failedMutation); - permanentFailures.add(failedMutation); - } + allFailures.add(failedMutation); + permanentFailures.add(failedMutation); + } - currentRequest = builder.build(); - originalIndexes = newOriginalIndexes; + currentRequest = builder.build(); + originalIndexes = newOriginalIndexes; - if (!allFailures.isEmpty()) { - boolean isRetryable = builder.getEntriesCount() > 0; - throw new MutateRowsException(null, allFailures, isRetryable); + if (!allFailures.isEmpty()) { + boolean isRetryable = builder.getEntriesCount() > 0; + throw new MutateRowsException(null, allFailures, isRetryable); + } } } From b91b251a2321b22e73c0593ed9048881cd95338a Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Tue, 12 Sep 2023 20:17:31 +0000 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20po?= =?UTF-8?q?st-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 2711efc50a..eb8e9b6391 100644 --- a/README.md +++ b/README.md @@ -57,13 +57,13 @@ implementation 'com.google.cloud:google-cloud-bigtable' If you are using Gradle without BOM, add this to your dependencies: ```Groovy -implementation 'com.google.cloud:google-cloud-bigtable:2.27.0' +implementation 'com.google.cloud:google-cloud-bigtable:2.27.1' ``` If you are using SBT, add this to your dependencies: ```Scala -libraryDependencies += "com.google.cloud" % "google-cloud-bigtable" % "2.27.0" +libraryDependencies += "com.google.cloud" % "google-cloud-bigtable" % "2.27.1" ``` @@ -609,7 +609,7 @@ Java is a registered trademark of Oracle and/or its affiliates. [kokoro-badge-link-5]: http://storage.googleapis.com/cloud-devrel-public/java/badges/java-bigtable/java11.html [stability-image]: https://img.shields.io/badge/stability-stable-green [maven-version-image]: https://img.shields.io/maven-central/v/com.google.cloud/google-cloud-bigtable.svg -[maven-version-link]: https://central.sonatype.com/artifact/com.google.cloud/google-cloud-bigtable/2.27.0 +[maven-version-link]: https://central.sonatype.com/artifact/com.google.cloud/google-cloud-bigtable/2.27.1 [authentication]: https://github.com/googleapis/google-cloud-java#authentication [auth-scopes]: https://developers.google.com/identity/protocols/oauth2/scopes [predefined-iam-roles]: https://cloud.google.com/iam/docs/understanding-roles#predefined_roles From 44cdefdb3348a470d4f7c79c9434a055b60c301d Mon Sep 17 00:00:00 2001 From: Igor Berntein Date: Tue, 12 Sep 2023 16:40:28 -0400 Subject: [PATCH 5/5] Revert "codestyle" This reverts commit 3d48bf5e36de06d16b98a058cf3698b0f160f228. --- .../mutaterows/MutateRowsAttemptCallable.java | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 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 4cb2b2a750..b049219a95 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 @@ -294,29 +294,30 @@ private void handleAttemptSuccess(List responses) { // Handle missing mutations for (int i = 0; i < seenIndices.length; i++) { - if (!seenIndices[i]) { + if (seenIndices[i]) { + continue; + } - int origIndex = getOriginalIndex(i); - FailedMutation failedMutation = - FailedMutation.create( - origIndex, - ApiExceptionFactory.createException( - "Missing entry response for entry " + origIndex, - null, - GrpcStatusCode.of(io.grpc.Status.Code.INTERNAL), - false)); + int origIndex = getOriginalIndex(i); + FailedMutation failedMutation = + FailedMutation.create( + origIndex, + ApiExceptionFactory.createException( + "Missing entry response for entry " + origIndex, + null, + GrpcStatusCode.of(io.grpc.Status.Code.INTERNAL), + false)); - allFailures.add(failedMutation); - permanentFailures.add(failedMutation); - } + allFailures.add(failedMutation); + permanentFailures.add(failedMutation); + } - currentRequest = builder.build(); - originalIndexes = newOriginalIndexes; + currentRequest = builder.build(); + originalIndexes = newOriginalIndexes; - if (!allFailures.isEmpty()) { - boolean isRetryable = builder.getEntriesCount() > 0; - throw new MutateRowsException(null, allFailures, isRetryable); - } + if (!allFailures.isEmpty()) { + boolean isRetryable = builder.getEntriesCount() > 0; + throw new MutateRowsException(null, allFailures, isRetryable); } }