Stop persisting last acknowledged batch ID (#2243)

After #2029, acknowledged batches are always immediately removed, so persisting the last acknowledged batch ID is no longer necessary.

Author: var-const

Firestore/Example/Tests/Local/FSTMutationQueueTests.mm

@@ -31,12 +31,14 @@
 #include "Firestore/core/src/firebase/firestore/auth/user.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key_set.h"
+#include "Firestore/core/src/firebase/firestore/model/mutation_batch.h"
 #include "Firestore/core/test/firebase/firestore/testutil/testutil.h"
 
 namespace testutil = firebase::firestore::testutil;
 using firebase::firestore::auth::User;
 using firebase::firestore::model::DocumentKey;
 using firebase::firestore::model::DocumentKeySet;
+using firebase::firestore::model::kBatchIdUnknown;
 using firebase::firestore::testutil::Key;
 
 NS_ASSUME_NONNULL_BEGIN
@@ -83,32 +85,30 @@ - (void)testCountBatches {
 - (void)testAcknowledgeBatchID {
   if ([self isTestBaseClass]) return;
 
-  // Initial state of an empty queue
-  XCTAssertEqual([self.mutationQueue highestAcknowledgedBatchID], kFSTBatchIDUnknown);
-
-  // Adding mutation batches should not change the highest acked batchID.
   self.persistence.run("testAcknowledgeBatchID", [&]() {
+    XCTAssertEqual([self batchCount], 0);
+
     FSTMutationBatch *batch1 = [self addMutationBatch];
     FSTMutationBatch *batch2 = [self addMutationBatch];
     FSTMutationBatch *batch3 = [self addMutationBatch];
-    XCTAssertGreaterThan(batch1.batchID, kFSTBatchIDUnknown);
+    XCTAssertGreaterThan(batch1.batchID, kBatchIdUnknown);
     XCTAssertGreaterThan(batch2.batchID, batch1.batchID);
     XCTAssertGreaterThan(batch3.batchID, batch2.batchID);
 
-    XCTAssertEqual([self.mutationQueue highestAcknowledgedBatchID], kFSTBatchIDUnknown);
+    XCTAssertEqual([self batchCount], 3);
 
     [self.mutationQueue acknowledgeBatch:batch1 streamToken:nil];
     [self.mutationQueue removeMutationBatch:batch1];
+    XCTAssertEqual([self batchCount], 2);
 
     [self.mutationQueue acknowledgeBatch:batch2 streamToken:nil];
-    XCTAssertEqual([self.mutationQueue highestAcknowledgedBatchID], batch2.batchID);
+    XCTAssertEqual([self batchCount], 2);
 
     [self.mutationQueue removeMutationBatch:batch2];
-    XCTAssertEqual([self.mutationQueue highestAcknowledgedBatchID], batch2.batchID);
+    XCTAssertEqual([self batchCount], 1);
 
-    // Batch 3 never acknowledged.
     [self.mutationQueue removeMutationBatch:batch3];
-    XCTAssertEqual([self.mutationQueue highestAcknowledgedBatchID], batch2.batchID);
+    XCTAssertEqual([self batchCount], 0);
   });
 }
 
@@ -122,7 +122,6 @@ - (void)testAcknowledgeThenRemove {
     [self.mutationQueue removeMutationBatch:batch1];
 
     XCTAssertEqual([self batchCount], 0);
-    XCTAssertEqual([self.mutationQueue highestAcknowledgedBatchID], batch1.batchID);
   });
 }
 
@@ -186,28 +185,6 @@ - (void)testNextMutationBatchAfterBatchID {
   });
 }
 
-- (void)testNextMutationBatchAfterBatchIDSkipsAcknowledgedBatches {
-  if ([self isTestBaseClass]) return;
-
-  NSMutableArray<FSTMutationBatch *> *batches = self.persistence.run(
-      "testNextMutationBatchAfterBatchIDSkipsAcknowledgedBatches newBatches",
-      [&]() -> NSMutableArray<FSTMutationBatch *> * {
-        NSMutableArray<FSTMutationBatch *> *newBatches = [self createBatches:3];
-        XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:kFSTBatchIDUnknown],
-                              newBatches[0]);
-        return newBatches;
-      });
-  self.persistence.run("testNextMutationBatchAfterBatchIDSkipsAcknowledgedBatches", [&]() {
-    [self.mutationQueue acknowledgeBatch:batches[0] streamToken:nil];
-    XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:kFSTBatchIDUnknown],
-                          batches[1]);
-    XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:batches[0].batchID],
-                          batches[1]);
-    XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:batches[1].batchID],
-                          batches[2]);
-  });
-}
-
 - (void)testAllMutationBatchesAffectingDocumentKey {
   if ([self isTestBaseClass]) return;
 
@@ -406,7 +383,6 @@ - (void)testStreamToken {
     XCTAssertEqualObjects([self.mutationQueue lastStreamToken], streamToken1);
 
     [self.mutationQueue acknowledgeBatch:batch1 streamToken:streamToken2];
-    XCTAssertEqual(self.mutationQueue.highestAcknowledgedBatchID, batch1.batchID);
     XCTAssertEqualObjects([self.mutationQueue lastStreamToken], streamToken2);
   });
 }

Firestore/Source/Local/FSTLevelDBMutationQueue.mm

@@ -33,6 +33,7 @@
 #include "Firestore/core/src/firebase/firestore/local/leveldb_key.h"
 #include "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h"
 #include "Firestore/core/src/firebase/firestore/local/leveldb_util.h"
+#include "Firestore/core/src/firebase/firestore/model/mutation_batch.h"
 #include "Firestore/core/src/firebase/firestore/model/resource_path.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
@@ -52,6 +53,7 @@
 using firebase::firestore::local::LevelDbTransaction;
 using firebase::firestore::local::MakeStringView;
 using firebase::firestore::model::BatchId;
+using firebase::firestore::model::kBatchIdUnknown;
 using firebase::firestore::model::DocumentKey;
 using firebase::firestore::model::DocumentKeySet;
 using firebase::firestore::model::ResourcePath;
@@ -115,31 +117,13 @@ - (instancetype)initWithUserID:(std::string)userID
 }
 
 - (void)start {
-  BatchId nextBatchID = [FSTLevelDBMutationQueue loadNextBatchIDFromDB:_db.ptr];
+  self.nextBatchID = [FSTLevelDBMutationQueue loadNextBatchIDFromDB:_db.ptr];
 
-  // On restart, nextBatchId may end up lower than lastAcknowledgedBatchId since it's computed from
-  // the queue contents, and there may be no mutations in the queue. In this case, we need to reset
-  // lastAcknowledgedBatchId (which is safe since the queue must be empty).
   std::string key = [self keyForCurrentMutationQueue];
   FSTPBMutationQueue *metadata = [self metadataForKey:key];
   if (!metadata) {
     metadata = [FSTPBMutationQueue message];
-
-    // proto3's default value for lastAcknowledgedBatchId is zero, but that would consider the first
-    // entry in the queue to be acknowledged without that acknowledgement actually happening.
-    metadata.lastAcknowledgedBatchId = kFSTBatchIDUnknown;
-  } else {
-    BatchId lastAcked = metadata.lastAcknowledgedBatchId;
-    if (lastAcked >= nextBatchID) {
-      HARD_ASSERT([self isEmpty], "Reset nextBatchID is only possible when the queue is empty");
-      lastAcked = kFSTBatchIDUnknown;
-
-      metadata.lastAcknowledgedBatchId = lastAcked;
-      _db.currentTransaction->Put([self keyForCurrentMutationQueue], metadata);
-    }
   }
-
-  self.nextBatchID = nextBatchID;
   self.metadata = metadata;
 }
 
@@ -151,7 +135,7 @@ + (BatchId)loadNextBatchIDFromDB:(DB *)db {
   auto tableKey = LevelDbMutationKey::KeyPrefix();
 
   LevelDbMutationKey rowKey;
-  BatchId maxBatchID = kFSTBatchIDUnknown;
+  BatchId maxBatchID = kBatchIdUnknown;
 
   BOOL moreUserIDs = NO;
   std::string nextUserID;
@@ -220,17 +204,8 @@ - (BOOL)isEmpty {
   return empty;
 }
 
-- (BatchId)highestAcknowledgedBatchID {
-  return self.metadata.lastAcknowledgedBatchId;
-}
-
 - (void)acknowledgeBatch:(FSTMutationBatch *)batch streamToken:(nullable NSData *)streamToken {
-  BatchId batchID = batch.batchID;
-  HARD_ASSERT(batchID > self.highestAcknowledgedBatchID,
-              "Mutation batchIDs must be acknowledged in order");
-
   FSTPBMutationQueue *metadata = self.metadata;
-  metadata.lastAcknowledgedBatchId = batchID;
   metadata.lastStreamToken = streamToken;
 
   _db.currentTransaction->Put([self keyForCurrentMutationQueue], metadata);
@@ -304,10 +279,7 @@ - (nullable FSTMutationBatch *)lookupMutationBatch:(BatchId)batchID {
 }
 
 - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(BatchId)batchID {
-  // All batches with batchID <= self.metadata.lastAcknowledgedBatchId have been acknowledged so
-  // the first unacknowledged batch after batchID will have a batchID larger than both of these
-  // values.
-  BatchId nextBatchID = MAX(batchID, self.metadata.lastAcknowledgedBatchId) + 1;
+  BatchId nextBatchID = batchID + 1;
 
   std::string key = [self mutationKeyForBatchID:nextBatchID];
   auto it = _db.currentTransaction->NewIterator();

Firestore/Source/Local/FSTLocalStore.mm

@@ -189,9 +189,6 @@ - (MaybeDocumentMap)rejectBatchID:(BatchId)batchID {
     FSTMutationBatch *toReject = [self.mutationQueue lookupMutationBatch:batchID];
     HARD_ASSERT(toReject, "Attempt to reject nonexistent batch!");
 
-    BatchId lastAcked = [self.mutationQueue highestAcknowledgedBatchID];
-    HARD_ASSERT(batchID > lastAcked, "Acknowledged batches can't be rejected.");
-
     [self.mutationQueue removeMutationBatch:toReject];
     [self.mutationQueue performConsistencyCheck];
 

Firestore/Source/Local/FSTMemoryMutationQueue.mm

@@ -68,9 +68,6 @@ @interface FSTMemoryMutationQueue ()
 /** The next value to use when assigning sequential IDs to each mutation batch. */
 @property(nonatomic, assign) BatchId nextBatchID;
 
-/** The highest acknowledged mutation in the queue. */
-@property(nonatomic, assign) BatchId highestAcknowledgedBatchID;
-
 /**
  * The last received stream token from the server, used to acknowledge which responses the client
  * has processed. Stream tokens are opaque checkpoint markers whose only real value is their
@@ -94,7 +91,6 @@ - (instancetype)initWithPersistence:(FSTMemoryPersistence *)persistence {
     _queue = [NSMutableArray array];
 
     _nextBatchID = 1;
-    _highestAcknowledgedBatchID = kFSTBatchIDUnknown;
   }
   return self;
 }
@@ -105,13 +101,10 @@ - (void)start {
   // Note: The queue may be shutdown / started multiple times, since we maintain the queue for the
   // duration of the app session in case a user logs out / back in. To behave like the
   // LevelDB-backed MutationQueue (and accommodate tests that expect as much), we reset nextBatchID
-  // and highestAcknowledgedBatchID if the queue is empty.
+  // if the queue is empty.
   if (self.isEmpty) {
     self.nextBatchID = 1;
-    self.highestAcknowledgedBatchID = kFSTBatchIDUnknown;
   }
-  HARD_ASSERT(self.highestAcknowledgedBatchID < self.nextBatchID,
-              "highestAcknowledgedBatchID must be less than the nextBatchID");
 }
 
 - (BOOL)isEmpty {
@@ -120,16 +113,10 @@ - (BOOL)isEmpty {
   return self.queue.count == 0;
 }
 
-- (BatchId)highestAcknowledgedBatchID {
-  return _highestAcknowledgedBatchID;
-}
-
 - (void)acknowledgeBatch:(FSTMutationBatch *)batch streamToken:(nullable NSData *)streamToken {
   NSMutableArray<FSTMutationBatch *> *queue = self.queue;
 
   BatchId batchID = batch.batchID;
-  HARD_ASSERT(batchID > self.highestAcknowledgedBatchID,
-              "Mutation batchIDs must be acknowledged in order");
 
   NSInteger batchIndex = [self indexOfExistingBatchID:batchID action:@"acknowledged"];
   HARD_ASSERT(batchIndex == 0, "Can only acknowledge the first batch in the mutation queue");
@@ -139,7 +126,6 @@ - (void)acknowledgeBatch:(FSTMutationBatch *)batch streamToken:(nullable NSData
   HARD_ASSERT(batchID == check.batchID, "Queue ordering failure: expected batch %s, got batch %s",
               batchID, check.batchID);
 
-  self.highestAcknowledgedBatchID = batchID;
   self.lastStreamToken = streamToken;
 }
 
@@ -186,9 +172,7 @@ - (nullable FSTMutationBatch *)lookupMutationBatch:(BatchId)batchID {
 - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(BatchId)batchID {
   NSMutableArray<FSTMutationBatch *> *queue = self.queue;
 
-  // All batches with batchID <= self.highestAcknowledgedBatchID have been acknowledged so the
-  // first unacknowledged batch after batchID will have a batchID larger than both of these values.
-  BatchId nextBatchID = MAX(batchID, self.highestAcknowledgedBatchID) + 1;
+  BatchId nextBatchID = batchID + 1;
 
   // The requested batchID may still be out of range so normalize it to the start of the queue.
   NSInteger rawIndex = [self indexOfBatchID:nextBatchID];

Firestore/Source/Local/FSTMutationQueue.h

@@ -35,22 +35,12 @@ NS_ASSUME_NONNULL_BEGIN
 /**
  * Starts the mutation queue, performing any initial reads that might be required to establish
  * invariants, etc.
- *
- * After starting, the mutation queue must guarantee that the highestAcknowledgedBatchID is less
- * than nextBatchID. This prevents the local store from creating new batches that the mutation
- * queue would consider erroneously acknowledged.
  */
 - (void)start;
 
 /** Returns YES if this queue contains no mutation batches. */
 - (BOOL)isEmpty;
 
-/**
- * Returns the highest batchID that has been acknowledged. If no batches have been acknowledged
- * or if there are no batches in the queue this can return kFSTBatchIDUnknown.
- */
-- (firebase::firestore::model::BatchId)highestAcknowledgedBatchID;
-
 /** Acknowledges the given batch. */
 - (void)acknowledgeBatch:(FSTMutationBatch *)batch streamToken:(nullable NSData *)streamToken;
 
@@ -71,7 +61,7 @@ NS_ASSUME_NONNULL_BEGIN
  * Gets the first unacknowledged mutation batch after the passed in batchId in the mutation queue
  * or nil if empty.
  *
- * @param batchID The batch to search after, or kFSTBatchIDUnknown for the first mutation in the
+ * @param batchID The batch to search after, or kBatchIdUnknown for the first mutation in the
  * queue.
  *
  * @return the next mutation or nil if there wasn't one.

Firestore/Source/Model/FSTMutationBatch.h

@@ -42,14 +42,6 @@ using DocumentVersionMap = std::unordered_map<DocumentKey, SnapshotVersion, Docu
 
 NS_ASSUME_NONNULL_BEGIN
 
-/**
- * A BatchID that was searched for and not found or a batch ID value known to be before all known
- * batches.
- *
- * BatchId values from the local store are non-negative so this value is before all batches.
- */
-extern const firebase::firestore::model::BatchId kFSTBatchIDUnknown;
-
 /**
  * A batch of mutations that will be sent as one unit to the backend. Batches can be marked as a
  * tombstone if the mutation queue does not remove them immediately. When a batch is a tombstone

Firestore/Source/Model/FSTMutationBatch.mm

@@ -34,8 +34,6 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-const BatchId kFSTBatchIDUnknown = -1;
-
 @implementation FSTMutationBatch
 
 - (instancetype)initWithBatchID:(BatchId)batchID

Firestore/Source/Remote/FSTRemoteStore.mm

@@ -35,6 +35,7 @@
 
 #include "Firestore/core/src/firebase/firestore/auth/user.h"
 #include "Firestore/core/src/firebase/firestore/model/document_key.h"
+#include "Firestore/core/src/firebase/firestore/model/mutation_batch.h"
 #include "Firestore/core/src/firebase/firestore/model/snapshot_version.h"
 #include "Firestore/core/src/firebase/firestore/remote/stream.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
@@ -45,6 +46,7 @@
 namespace util = firebase::firestore::util;
 using firebase::firestore::auth::User;
 using firebase::firestore::model::BatchId;
+using firebase::firestore::model::kBatchIdUnknown;
 using firebase::firestore::model::DocumentKey;
 using firebase::firestore::model::DocumentKeySet;
 using firebase::firestore::model::OnlineState;
@@ -464,7 +466,7 @@ - (void)startWriteStream {
  */
 - (void)fillWritePipeline {
   BatchId lastBatchIDRetrieved =
-      self.writePipeline.count == 0 ? kFSTBatchIDUnknown : self.writePipeline.lastObject.batchID;
+      self.writePipeline.count == 0 ? kBatchIdUnknown : self.writePipeline.lastObject.batchID;
   while ([self canAddToWritePipeline]) {
     FSTMutationBatch *batch = [self.localStore nextMutationBatchAfterBatchID:lastBatchIDRetrieved];
     if (!batch) {

Firestore/core/src/firebase/firestore/model/mutation_batch.h

@@ -0,0 +1,39 @@
+/*
+ * Copyright 2019 Google
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_MUTATION_BATCH_H_
+#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_MUTATION_BATCH_H_
+
+#include "Firestore/core/src/firebase/firestore/model/types.h"
+
+namespace firebase {
+namespace firestore {
+namespace model {
+
+/**
+ * A BatchID that was searched for and not found or a batch ID value known to be
+ * before all known batches.
+ *
+ * BatchId values from the local store are non-negative so this value is before
+ * all batches.
+ */
+constexpr BatchId kBatchIdUnknown = -1;
+
+}  // namespace model
+}  // namespace firestore
+}  // namespace firebase
+
+#endif  // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_MUTATION_BATCH_H_