Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 9ad8a00

Browse files
authoredSep 12, 2023
fix: check that all bulk mutation entries are accounted for (#1907)
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 Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://togithub.com/googleapis/java-bigtable/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) Fixes #<issue_number_goes_here> ☕️ If you write sample code, please follow the [samples format]( https://togithub.com/GoogleCloudPlatform/java-docs-samples/blob/main/SAMPLE_FORMAT.md).
1 parent 6208c90 commit 9ad8a00

File tree

7 files changed

+83
-5
lines changed

7 files changed

+83
-5
lines changed
 

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallable.java

+24
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import com.google.common.base.Preconditions;
3636
import com.google.common.collect.ImmutableList;
3737
import com.google.common.collect.Lists;
38+
import com.google.common.primitives.Ints;
3839
import com.google.common.util.concurrent.MoreExecutors;
3940
import com.google.rpc.Code;
4041
import java.util.List;
@@ -263,9 +264,12 @@ private void handleAttemptSuccess(List<MutateRowsResponse> responses) {
263264

264265
Builder builder = lastRequest.toBuilder().clearEntries();
265266
List<Integer> newOriginalIndexes = Lists.newArrayList();
267+
boolean[] seenIndices = new boolean[currentRequest.getEntriesCount()];
266268

267269
for (MutateRowsResponse response : responses) {
268270
for (Entry entry : response.getEntriesList()) {
271+
seenIndices[Ints.checkedCast(entry.getIndex())] = true;
272+
269273
if (entry.getStatus().getCode() == Code.OK_VALUE) {
270274
continue;
271275
}
@@ -288,6 +292,26 @@ private void handleAttemptSuccess(List<MutateRowsResponse> responses) {
288292
}
289293
}
290294

295+
// Handle missing mutations
296+
for (int i = 0; i < seenIndices.length; i++) {
297+
if (seenIndices[i]) {
298+
continue;
299+
}
300+
301+
int origIndex = getOriginalIndex(i);
302+
FailedMutation failedMutation =
303+
FailedMutation.create(
304+
origIndex,
305+
ApiExceptionFactory.createException(
306+
"Missing entry response for entry " + origIndex,
307+
null,
308+
GrpcStatusCode.of(io.grpc.Status.Code.INTERNAL),
309+
false));
310+
311+
allFailures.add(failedMutation);
312+
permanentFailures.add(failedMutation);
313+
}
314+
291315
currentRequest = builder.build();
292316
originalIndexes = newOriginalIndexes;
293317

‎google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerCallableTest.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,11 @@ public void mutateRow(MutateRowRequest request, StreamObserver<MutateRowResponse
403403

404404
@Override
405405
public void mutateRows(MutateRowsRequest request, StreamObserver<MutateRowsResponse> observer) {
406-
observer.onNext(MutateRowsResponse.getDefaultInstance());
406+
MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder();
407+
for (int i = 0; i < request.getEntriesCount(); i++) {
408+
builder.addEntries(MutateRowsResponse.Entry.newBuilder().setIndex(i));
409+
}
410+
observer.onNext(builder.build());
407411
observer.onCompleted();
408412
}
409413

‎google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -648,7 +648,11 @@ public void mutateRows(
648648
Thread.sleep(SERVER_LATENCY);
649649
} catch (InterruptedException e) {
650650
}
651-
responseObserver.onNext(MutateRowsResponse.getDefaultInstance());
651+
MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder();
652+
for (int i = 0; i < request.getEntriesCount(); i++) {
653+
builder.addEntriesBuilder().setIndex(i);
654+
}
655+
responseObserver.onNext(builder.build());
652656
responseObserver.onCompleted();
653657
}
654658

‎google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java

+6-1
Original file line numberDiff line numberDiff line change
@@ -422,10 +422,15 @@ public void testBatchMutateRowsThrottledTime() throws Exception {
422422
new Answer() {
423423
@Override
424424
public Object answer(InvocationOnMock invocation) {
425+
MutateRowsRequest request = (MutateRowsRequest) invocation.getArguments()[0];
425426
@SuppressWarnings("unchecked")
426427
StreamObserver<MutateRowsResponse> observer =
427428
(StreamObserver<MutateRowsResponse>) invocation.getArguments()[1];
428-
observer.onNext(MutateRowsResponse.getDefaultInstance());
429+
MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder();
430+
for (int i = 0; i < request.getEntriesCount(); i++) {
431+
builder.addEntriesBuilder().setIndex(i);
432+
}
433+
observer.onNext(builder.build());
429434
observer.onCompleted();
430435
return null;
431436
}

‎google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/StatsHeadersCallableTest.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,11 @@ public void mutateRows(MutateRowsRequest request, StreamObserver<MutateRowsRespo
223223
observer.onError(new StatusRuntimeException(Status.UNAVAILABLE));
224224
return;
225225
}
226-
observer.onNext(MutateRowsResponse.getDefaultInstance());
226+
MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder();
227+
for (int i = 0; i < request.getEntriesCount(); i++) {
228+
builder.addEntriesBuilder().setIndex(i);
229+
}
230+
observer.onNext(builder.build());
227231
observer.onCompleted();
228232
}
229233

‎google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsAttemptCallableTest.java

+33
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@
4141
import java.util.List;
4242
import java.util.Set;
4343
import java.util.concurrent.Callable;
44+
import java.util.concurrent.ExecutionException;
45+
import org.junit.Assert;
4446
import org.junit.Before;
4547
import org.junit.Test;
4648
import org.junit.runner.RunWith;
@@ -92,6 +94,37 @@ public void singleEntrySuccessTest() throws Exception {
9294
assertThat(innerCallable.lastRequest).isEqualTo(request);
9395
}
9496

97+
@Test
98+
public void missingEntry() {
99+
MutateRowsRequest request =
100+
MutateRowsRequest.newBuilder()
101+
.addEntries(Entry.getDefaultInstance())
102+
.addEntries(Entry.getDefaultInstance())
103+
.build();
104+
innerCallable.response.add(
105+
MutateRowsResponse.newBuilder()
106+
.addEntries(MutateRowsResponse.Entry.newBuilder().setIndex(0))
107+
.build());
108+
109+
MutateRowsAttemptCallable attemptCallable =
110+
new MutateRowsAttemptCallable(innerCallable, request, callContext, retryCodes);
111+
attemptCallable.setExternalFuture(parentFuture);
112+
attemptCallable.call();
113+
114+
ExecutionException executionException =
115+
Assert.assertThrows(ExecutionException.class, () -> parentFuture.attemptFuture.get());
116+
assertThat(executionException).hasCauseThat().isInstanceOf(MutateRowsException.class);
117+
MutateRowsException e = (MutateRowsException) executionException.getCause();
118+
119+
assertThat(e).hasMessageThat().contains("Some mutations failed to apply");
120+
assertThat(e.getFailedMutations()).hasSize(1);
121+
FailedMutation failedMutation = e.getFailedMutations().get(0);
122+
assertThat(failedMutation.getIndex()).isEqualTo(1);
123+
assertThat(failedMutation.getError())
124+
.hasMessageThat()
125+
.contains("Missing entry response for entry 1");
126+
}
127+
95128
@Test
96129
public void testNoRpcTimeout() {
97130
parentFuture.timedAttemptSettings =

‎google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/mutaterows/MutateRowsRetryTest.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,11 @@ public void mutateRows(
107107
MutateRowsRequest request, StreamObserver<MutateRowsResponse> responseObserver) {
108108
attemptCounter.incrementAndGet();
109109
if (expectations.isEmpty()) {
110-
responseObserver.onNext(MutateRowsResponse.getDefaultInstance());
110+
MutateRowsResponse.Builder builder = MutateRowsResponse.newBuilder();
111+
for (int i = 0; i < request.getEntriesCount(); i++) {
112+
builder.addEntriesBuilder().setIndex(i);
113+
}
114+
responseObserver.onNext(builder.build());
111115
responseObserver.onCompleted();
112116
} else {
113117
Exception expectedRpc = expectations.poll();

0 commit comments

Comments
 (0)
Failed to load comments.