From 2d9b4d4ba976531a04b21bd1e42a2e2f0dc4a346 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Fri, 25 Jun 2021 14:39:22 -0700 Subject: [PATCH 1/2] standardize transaction retries to attempts --- .../firebase/firestore/TransactionTest.java | 21 +++++++++++++++++++ .../firestore/core/TransactionRunner.java | 10 ++++----- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java index 0f084f60bb9..5f02986bb72 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java @@ -29,6 +29,7 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.firestore.FirebaseFirestoreException.Code; +import com.google.firebase.firestore.core.TransactionRunner; import com.google.firebase.firestore.testutil.IntegrationTestUtil; import com.google.firebase.firestore.util.AsyncQueue.TimerId; import java.util.ArrayList; @@ -632,6 +633,26 @@ public void testDoesNotRetryOnPermanentError() { assertEquals(1, count.get()); } + @Test + public void testMakesDefaultMaxAttempts() { + FirebaseFirestore firestore = testFirestore(); + DocumentReference doc1 = firestore.collection("counters").document(); + AtomicInteger count = new AtomicInteger(0); + waitFor(doc1.set(map("count", 15))); + Task transactionTask = firestore.runTransaction( + transaction -> { + // Get the first doc. + transaction.get(doc1); + // Do a write outside of the transaction to cause the transaction to fail. + waitFor(doc1.set(map("count", 1234 + count.incrementAndGet()))); + return null; + }); + + Exception e = waitForException(transactionTask); + assertEquals(Code.FAILED_PRECONDITION, ((FirebaseFirestoreException) e).getCode()); + assertEquals(TransactionRunner.DEFAULT_MAX_ATTEMPTS_COUNT, count.get()); + } + @Test public void testSuccessWithNoTransactionOperations() { FirebaseFirestore firestore = testFirestore(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TransactionRunner.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TransactionRunner.java index 2adf95b5213..7dd50b36ca4 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TransactionRunner.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TransactionRunner.java @@ -27,11 +27,11 @@ /** TransactionRunner encapsulates the logic needed to run and retry transactions with backoff. */ public class TransactionRunner { - private static final int RETRY_COUNT = 5; + public static final int DEFAULT_MAX_ATTEMPTS_COUNT = 5; private AsyncQueue asyncQueue; private RemoteStore remoteStore; private Function> updateFunction; - private int retriesLeft; + private int attemptsRemaining; private ExponentialBackoff backoff; private TaskCompletionSource taskSource = new TaskCompletionSource<>(); @@ -44,7 +44,7 @@ public TransactionRunner( this.asyncQueue = asyncQueue; this.remoteStore = remoteStore; this.updateFunction = updateFunction; - this.retriesLeft = RETRY_COUNT; + this.attemptsRemaining = DEFAULT_MAX_ATTEMPTS_COUNT; backoff = new ExponentialBackoff(asyncQueue, TimerId.RETRY_TRANSACTION); } @@ -56,6 +56,7 @@ public Task run() { } private void runWithBackoff() { + attemptsRemaining -= 1; backoff.backoffAndRun( () -> { final Transaction transaction = remoteStore.createTransaction(); @@ -84,8 +85,7 @@ private void runWithBackoff() { } private void handleTransactionError(Task task) { - if (retriesLeft > 0 && isRetryableTransactionError(task.getException())) { - retriesLeft -= 1; + if (attemptsRemaining > 0 && isRetryableTransactionError(task.getException())) { runWithBackoff(); } else { taskSource.setException(task.getException()); From 822475c059fcc0093e42e6139df343f3ff0d720f Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Fri, 25 Jun 2021 14:50:43 -0700 Subject: [PATCH 2/2] lint --- .../firebase/firestore/TransactionTest.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java index 5f02986bb72..23c74406068 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java @@ -639,14 +639,15 @@ public void testMakesDefaultMaxAttempts() { DocumentReference doc1 = firestore.collection("counters").document(); AtomicInteger count = new AtomicInteger(0); waitFor(doc1.set(map("count", 15))); - Task transactionTask = firestore.runTransaction( - transaction -> { - // Get the first doc. - transaction.get(doc1); - // Do a write outside of the transaction to cause the transaction to fail. - waitFor(doc1.set(map("count", 1234 + count.incrementAndGet()))); - return null; - }); + Task transactionTask = + firestore.runTransaction( + transaction -> { + // Get the first doc. + transaction.get(doc1); + // Do a write outside of the transaction to cause the transaction to fail. + waitFor(doc1.set(map("count", 1234 + count.incrementAndGet()))); + return null; + }); Exception e = waitForException(transactionTask); assertEquals(Code.FAILED_PRECONDITION, ((FirebaseFirestoreException) e).getCode());