From 8c851d8828760cc4cac8e0a71bf6c45d8e4a075e Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 12 Feb 2024 12:25:34 -0500 Subject: [PATCH] Feedback --- .../google/cloud/firestore/FirestoreImpl.java | 20 ++++++++++++++++-- .../cloud/firestore/ReadTimeTransaction.java | 21 ++++++++++--------- .../firestore/ServerSideTransaction.java | 9 ++++++++ .../ServerSideTransactionRunner.java | 8 +++++-- .../cloud/firestore/it/ITSystemTest.java | 6 +++++- 5 files changed, 49 insertions(+), 15 deletions(-) diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index 9d25519d2..82dfc5176 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -194,14 +194,14 @@ public Iterable listCollections() { @Override public ApiFuture> getAll( @Nonnull DocumentReference... documentReferences) { - return this.getAll(documentReferences, null, (ByteString) null, null); + return this.getAll(documentReferences, null, (ByteString) null); } @Nonnull @Override public ApiFuture> getAll( @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask) { - return this.getAll(documentReferences, fieldMask, (ByteString) null, null); + return this.getAll(documentReferences, fieldMask, (ByteString) null); } @Override @@ -323,6 +323,20 @@ public void onComplete() { streamRequest(request.build(), responseObserver, firestoreClient.batchGetDocumentsCallable()); } + final ApiFuture> getAll( + final @Nonnull DocumentReference[] documentReferences, + @Nullable FieldMask fieldMask, + @Nullable com.google.protobuf.Timestamp readTime) { + return getAll(documentReferences, fieldMask, null, readTime); + } + + private ApiFuture> getAll( + final @Nonnull DocumentReference[] documentReferences, + @Nullable FieldMask fieldMask, + @Nullable ByteString transactionId) { + return getAll(documentReferences, fieldMask, transactionId, null); + } + /** Internal getAll() method that accepts an optional transaction id. */ ApiFuture> getAll( final @Nonnull DocumentReference[] documentReferences, @@ -398,6 +412,8 @@ public ApiFuture runAsyncTransaction( @Nonnull TransactionOptions transactionOptions) { if (transactionOptions.getReadTime() != null) { + // READ_ONLY transactions with readTime have no retry, nor transaction state, so we don't need + // a runner. return updateFunction.updateCallback( new ReadTimeTransaction(this, transactionOptions.getReadTime())); } else { diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java index c7d0eab69..36f86f4d1 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java @@ -20,7 +20,6 @@ import com.google.api.core.ApiFutures; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; -import com.google.protobuf.ByteString; import com.google.protobuf.Timestamp; import io.opencensus.trace.Tracing; import java.util.List; @@ -28,6 +27,14 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; +/** + * The ReadTimeTransaction is a ready-only Transaction that specifies a ReadTime. Unlike a + * `ServerSideTransaction`, we do not need a `transactionId` since we provide a `readTime` on all + * requests. No concurrency control is required, since data in the past is immutable. As with all + * `read-only` transactions, we do not allow any write request. + * + * @see Transaction + */ final class ReadTimeTransaction extends Transaction { public static final String WRITE_EXCEPTION_MSG = @@ -50,11 +57,7 @@ public boolean hasTransactionId() { public ApiFuture get(@Nonnull DocumentReference documentRef) { Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_GETDOCUMENT); return ApiFutures.transform( - firestore.getAll( - new DocumentReference[] {documentRef}, - /*fieldMask=*/ null, - /*transactionId=*/ (ByteString) null, - readTime), + firestore.getAll(new DocumentReference[] {documentRef}, /*fieldMask=*/ null, readTime), snapshots -> snapshots.isEmpty() ? null : snapshots.get(0), MoreExecutors.directExecutor()); } @@ -63,16 +66,14 @@ public ApiFuture get(@Nonnull DocumentReference documentRef) { @Override public ApiFuture> getAll( @Nonnull DocumentReference... documentReferences) { - return firestore.getAll( - documentReferences, /*fieldMask=*/ null, /*transactionId=*/ (ByteString) null, readTime); + return firestore.getAll(documentReferences, /*fieldMask=*/ null, readTime); } @Nonnull @Override public ApiFuture> getAll( @Nonnull DocumentReference[] documentReferences, @Nullable FieldMask fieldMask) { - return firestore.getAll( - documentReferences, /*fieldMask=*/ null, /*transactionId=*/ (ByteString) null, readTime); + return firestore.getAll(documentReferences, /*fieldMask=*/ null, readTime); } @Nonnull diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java index b935dc3b4..0227c2c4c 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransaction.java @@ -34,6 +34,15 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; +/** + * A `ServerSideTransaction` is a `Transaction` that uses server generated `transactionId` on + * requests. The implementation starts with a `beginTransaction` request that receives a + * `transactionId` from server. The `ServerSideTransactionRunner` must either `commit()` or + * `rollback()` when done. + * + * @see Transaction + * @see ServerSideTransactionRunner + */ final class ServerSideTransaction extends Transaction { private static final Logger LOGGER = Logger.getLogger(ServerSideTransaction.class.getName()); diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java index c05ca76f1..5cf4c1f79 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java @@ -115,7 +115,11 @@ ApiFuture begin() { } private ApiFuture maybeRollback() { - return transaction != null ? transaction.rollback() : ApiFutures.immediateFuture(null); + return hasTransaction() ? transaction.rollback() : ApiFutures.immediateFuture(null); + } + + private boolean hasTransaction() { + return transaction != null; } /** A callback that invokes the BeginTransaction callback. */ @@ -251,7 +255,7 @@ private static boolean isRetryableTransactionError(ApiException exception) { private ApiFuture rollbackAndReject(final Throwable throwable) { final SettableApiFuture failedTransaction = SettableApiFuture.create(); - if (transaction != null) { + if (hasTransaction()) { // We use `addListener()` since we want to return the original exception regardless of // whether rollback() succeeds. transaction diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java index 6c9f55a99..3766800a5 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java @@ -1841,7 +1841,11 @@ public void readOnlyTransaction_failureWhenAttemptReadOlderThan60Seconds() // To ensure we exceed this, we use 120 minutes. // If this test fails, we should likely be update documentation to reflect new value. See all // usages of "Read Time" on proto, and within SDK. - final long twoHours = System.currentTimeMillis() / 1000 - 7200; + // + // If Point-in-Time Recovery is enabled, can additionally be a whole minute timestamp within the + // past 7 days. For that reason `twoHours` is calculated to whole minute to more accurately + // catch this situation. + final long twoHours = (System.currentTimeMillis() / 60_000 - 120) * 60; final TransactionOptions options = TransactionOptions.createReadOnlyOptionsBuilder() .setReadTime(