Skip to content

Commit 4c39af5

Browse files
tom-andersentom-andersen
authored
fix: Make rollback best effort. (#1515)
* fix: Make rollback best effort. * Pretty * Fix from review
1 parent 3c78fe3 commit 4c39af5

File tree

3 files changed

+53
-33
lines changed

3 files changed

+53
-33
lines changed

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java‎

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@
2929
import com.google.protobuf.Empty;
3030
import io.opencensus.trace.Tracing;
3131
import java.util.List;
32+
import java.util.logging.Level;
33+
import java.util.logging.Logger;
3234
import javax.annotation.Nonnull;
3335
import javax.annotation.Nullable;
3436

@@ -40,6 +42,7 @@
4042
*/
4143
public final class Transaction extends UpdateBuilder<Transaction> {
4244

45+
private static final Logger LOGGER = Logger.getLogger(Transaction.class.getName());
4346
private static final String READ_BEFORE_WRITE_ERROR_MSG =
4447
"Firestore transactions require all reads to be executed before all writes";
4548

@@ -121,15 +124,29 @@ ApiFuture<List<WriteResult>> commit() {
121124
/** Rolls a transaction back and releases all read locks. */
122125
ApiFuture<Void> rollback() {
123126
Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_ROLLBACK);
124-
RollbackRequest.Builder reqBuilder = RollbackRequest.newBuilder();
125-
reqBuilder.setTransaction(transactionId);
126-
reqBuilder.setDatabase(firestore.getDatabaseName());
127+
RollbackRequest req =
128+
RollbackRequest.newBuilder()
129+
.setTransaction(transactionId)
130+
.setDatabase(firestore.getDatabaseName())
131+
.build();
127132

128133
ApiFuture<Empty> rollbackFuture =
129-
firestore.sendRequest(reqBuilder.build(), firestore.getClient().rollbackCallable());
130-
131-
return ApiFutures.transform(
132-
rollbackFuture, beginTransactionResponse -> null, MoreExecutors.directExecutor());
134+
firestore.sendRequest(req, firestore.getClient().rollbackCallable());
135+
136+
ApiFuture<Void> transform =
137+
ApiFutures.transform(rollbackFuture, resp -> null, MoreExecutors.directExecutor());
138+
139+
return ApiFutures.catching(
140+
transform,
141+
Throwable.class,
142+
(error) -> {
143+
LOGGER.log(
144+
Level.WARNING,
145+
"Failed best effort to rollback of transaction " + transactionId,
146+
error);
147+
return null;
148+
},
149+
MoreExecutors.directExecutor());
133150
}
134151

135152
/**

‎google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package com.google.cloud.firestore;
1818

19+
import static org.junit.Assert.assertArrayEquals;
1920
import static org.junit.Assert.assertEquals;
2021
import static org.mockito.Mockito.doAnswer;
2122
import static org.mockito.Mockito.mock;
@@ -83,6 +84,7 @@
8384
import java.util.List;
8485
import java.util.Map;
8586
import java.util.Objects;
87+
import java.util.concurrent.CopyOnWriteArrayList;
8688
import java.util.concurrent.TimeUnit;
8789
import javax.annotation.Nullable;
8890
import org.mockito.ArgumentCaptor;
@@ -1195,10 +1197,10 @@ public RequestResponsePair(
11951197
* `sendRequest()` is called.
11961198
*/
11971199
static class ResponseStubber {
1198-
int requestCount = 0;
1199-
12001200
List<RequestResponsePair> operationList = new ArrayList<>();
12011201

1202+
List<Object> actualRequestList = new CopyOnWriteArrayList<>();
1203+
12021204
void put(GeneratedMessageV3 request, ApiFuture<? extends GeneratedMessageV3> response) {
12031205
operationList.add(new RequestResponsePair(request, response));
12041206
}
@@ -1209,8 +1211,7 @@ void initializeStub(
12091211
for (final RequestResponsePair entry : operationList) {
12101212
Answer<ApiFuture<? extends GeneratedMessageV3>> answer =
12111213
invocationOnMock -> {
1212-
++requestCount;
1213-
assertEquals(entry.request, invocationOnMock.getArguments()[0]);
1214+
actualRequestList.add(invocationOnMock.getArguments()[0]);
12141215
return entry.response;
12151216
};
12161217
stubber = (stubber != null) ? stubber.doAnswer(answer) : doAnswer(answer);
@@ -1223,10 +1224,10 @@ void initializeStub(
12231224
}
12241225

12251226
public void verifyAllRequestsSent() {
1226-
assertEquals(
1227-
String.format("Expected %d requests, but got %d", operationList.size(), requestCount),
1228-
operationList.size(),
1229-
requestCount);
1227+
assertArrayEquals(
1228+
"Expected requests, but got actual requests",
1229+
operationList.stream().map(x -> x.request).toArray(),
1230+
actualRequestList.toArray());
12301231
}
12311232
}
12321233

‎google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package com.google.cloud.firestore;
1818

19+
import static com.google.api.core.ApiFutures.immediateFailedFuture;
1920
import static com.google.cloud.firestore.LocalFirestoreHelper.IMMEDIATE_RETRY_SETTINGS;
2021
import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_FIELD_PROTO;
2122
import static com.google.cloud.firestore.LocalFirestoreHelper.TRANSACTION_ID;
@@ -88,7 +89,7 @@
8889
public class TransactionTest {
8990

9091
private final ApiFuture<GeneratedMessageV3> RETRYABLE_API_EXCEPTION =
91-
ApiFutures.immediateFailedFuture(
92+
immediateFailedFuture(
9293
new ApiException(
9394
new Exception("Test exception"), GrpcStatusCode.of(Status.Code.UNKNOWN), true));
9495

@@ -244,7 +245,7 @@ public void rollbackOnCallbackApiFutureErrorAsync() {
244245

245246
ApiFuture<String> transaction =
246247
firestoreMock.runAsyncTransaction(
247-
t -> ApiFutures.immediateFailedFuture(new Exception("Expected exception")), options);
248+
t -> immediateFailedFuture(new Exception("Expected exception")), options);
248249

249250
try {
250251
transaction.get();
@@ -262,7 +263,7 @@ public void rollbackOnCallbackApiFutureErrorAsync() {
262263

263264
@Test
264265
public void noRollbackOnBeginFailure() {
265-
doReturn(ApiFutures.immediateFailedFuture(new Exception("Expected exception")))
266+
doReturn(immediateFailedFuture(new Exception("Expected exception")))
266267
.when(firestoreMock)
267268
.sendRequest(
268269
requestCapture.capture(), ArgumentMatchers.<UnaryCallable<Message, Message>>any());
@@ -288,7 +289,7 @@ public void noRollbackOnBeginFailure() {
288289

289290
@Test
290291
public void noRollbackOnBeginFailureAsync() {
291-
doReturn(ApiFutures.immediateFailedFuture(new Exception("Expected exception")))
292+
doReturn(immediateFailedFuture(new Exception("Expected exception")))
292293
.when(firestoreMock)
293294
.sendRequest(
294295
requestCapture.capture(), ArgumentMatchers.<UnaryCallable<Message, Message>>any());
@@ -493,7 +494,7 @@ public void retriesCommitBasedOnErrorCode() throws Exception {
493494
new ResponseStubber() {
494495
{
495496
put(begin(), beginResponse("foo1"));
496-
put(commit("foo1"), ApiFutures.immediateFailedFuture(e));
497+
put(commit("foo1"), immediateFailedFuture(e));
497498
put(rollback("foo1"), rollbackResponse());
498499
put(begin("foo1"), beginResponse("foo2"));
499500
put(commit("foo2"), commitResponse(0, 0));
@@ -503,40 +504,41 @@ public void retriesCommitBasedOnErrorCode() throws Exception {
503504
new ResponseStubber() {
504505
{
505506
put(begin(), beginResponse("foo1"));
506-
put(commit("foo1"), ApiFutures.immediateFailedFuture(e));
507+
put(commit("foo1"), immediateFailedFuture(e));
507508
put(rollback("foo1"), rollbackResponse());
508509
}
509510
});
510511
}
511512

512513
@Test
513514
public void retriesRollbackBasedOnErrorCode() throws Exception {
514-
final ApiException commitException = exception(Status.Code.ABORTED, true);
515-
515+
final ApiException retryable = exception(Status.Code.ABORTED, true);
516+
517+
// Regardless of exception thrown by rollback, we should never retry
518+
// calling rollback. Rollback is best effort, and will sometimes return
519+
// ABORT error (which a transaction will retry) when transaction no longer
520+
// exists on Firestore server side. Attempting to retry will in some cases
521+
// simply exhaust retries with accumulated backoff delay, when a new
522+
// transaction could simply be started (since the old transaction no longer
523+
// exists server side).
516524
verifyRetries(
517525
/* expectedSequenceWithRetry= */ e -> {
518-
final ApiFuture<GeneratedMessageV3> rollbackException =
519-
ApiFutures.immediateFailedFuture(e);
520526
return new ResponseStubber() {
521527
{
522528
put(begin(), beginResponse("foo1"));
523-
put(commit("foo1"), ApiFutures.immediateFailedFuture(commitException));
524-
put(rollback("foo1"), rollbackException);
525-
put(rollback("foo1"), rollbackResponse());
529+
put(commit("foo1"), immediateFailedFuture(e));
530+
put(rollback("foo1"), immediateFailedFuture(retryable));
526531
put(begin("foo1"), beginResponse("foo2"));
527532
put(commit("foo2"), commitResponse(0, 0));
528533
}
529534
};
530535
},
531536
/* expectedSequenceWithoutRetry= */ e -> {
532-
final ApiFuture<GeneratedMessageV3> rollbackException =
533-
ApiFutures.immediateFailedFuture(e);
534537
return new ResponseStubber() {
535538
{
536539
put(begin(), beginResponse("foo1"));
537-
put(commit("foo1"), ApiFutures.immediateFailedFuture(commitException));
538-
put(rollback("foo1"), rollbackException);
539-
put(rollback("foo1"), rollbackResponse());
540+
put(commit("foo1"), immediateFailedFuture(e));
541+
put(rollback("foo1"), immediateFailedFuture(retryable));
540542
}
541543
};
542544
});

0 commit comments

Comments
 (0)