Feedback

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

@@ -194,14 +194,14 @@ public Iterable<CollectionReference> listCollections() {
   @Override
   public ApiFuture<List<DocumentSnapshot>> getAll(
       @Nonnull DocumentReference... documentReferences) {
-    return this.getAll(documentReferences, null, (ByteString) null, null);
+    return this.getAll(documentReferences, null, (ByteString) null);
   }
 
   @Nonnull
   @Override
   public ApiFuture<List<DocumentSnapshot>> 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<List<DocumentSnapshot>> getAll(
+      final @Nonnull DocumentReference[] documentReferences,
+      @Nullable FieldMask fieldMask,
+      @Nullable com.google.protobuf.Timestamp readTime) {
+    return getAll(documentReferences, fieldMask, null, readTime);
+  }
+
+  private ApiFuture<List<DocumentSnapshot>> 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<List<DocumentSnapshot>> getAll(
       final @Nonnull DocumentReference[] documentReferences,
@@ -398,6 +412,8 @@ public <T> ApiFuture<T> 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 {

google-cloud-firestore/src/main/java/com/google/cloud/firestore/ReadTimeTransaction.java

@@ -20,14 +20,21 @@
 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;
 import java.util.Map;
 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<DocumentSnapshot> 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<DocumentSnapshot> get(@Nonnull DocumentReference documentRef) {
   @Override
   public ApiFuture<List<DocumentSnapshot>> 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<List<DocumentSnapshot>> 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

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());

google-cloud-firestore/src/main/java/com/google/cloud/firestore/ServerSideTransactionRunner.java

@@ -115,7 +115,11 @@ ApiFuture<ServerSideTransaction> begin() {
   }
 
   private ApiFuture<Void> 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<T> rollbackAndReject(final Throwable throwable) {
     final SettableApiFuture<T> 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

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(