Stop persisting last acknowledged batch ID #2243

var-const · 2019-01-09T22:22:00Z

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

var-const · 2019-01-09T22:23:01Z

Firestore/Example/Tests/Local/FSTMutationQueueTests.mm

    FSTMutationBatch *batch1 = [self addMutationBatch];
    FSTMutationBatch *batch2 = [self addMutationBatch];
    FSTMutationBatch *batch3 = [self addMutationBatch];
    XCTAssertGreaterThan(batch1.batchID, kFSTBatchIDUnknown);
    XCTAssertGreaterThan(batch2.batchID, batch1.batchID);
    XCTAssertGreaterThan(batch3.batchID, batch2.batchID);

-    XCTAssertEqual([self.mutationQueue highestAcknowledgedBatchID], kFSTBatchIDUnknown);
+    XCTAssertEqual([self batchCount], 3);


Checking batchCount is the only replacement for the previous check that I could think of.

var-const · 2019-01-09T22:23:28Z

Firestore/Example/Tests/Local/FSTMutationQueueTests.mm

@@ -186,28 +183,6 @@ - (void)testNextMutationBatchAfterBatchID {
  });
 }

- (void)testNextMutationBatchAfterBatchIDSkipsAcknowledgedBatches {


This test appears to be no longer relevant.

Right. The mutation queue now contains only unacknowledged writes now.

var-const · 2019-01-09T22:24:58Z

Firestore/Source/Local/FSTLevelDBMutationQueue.mm

-  // 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;


I presume this is fine?

wilhuff

LGTM for the parts that are here.

What do you think of adding a migration that forces the stored value back to the unknown batch ID?

var-const

Took a stab at doing a migration, please take a look. Given that the effects of the migration are invisible after the changes made in this PR, I'm not sure it's worthwhile, though.

var-const · 2019-01-10T20:42:08Z

Firestore/Source/Local/FSTLevelDBMutationQueue.mm

- (BatchId)highestAcknowledgedBatchID {
-  return self.metadata.lastAcknowledgedBatchId;
-}
-
 - (void)acknowledgeBatch:(FSTMutationBatch *)batch streamToken:(nullable NSData *)streamToken {


Note that this function is now equivalent to setLastStreamToken.

wilhuff

Basically LGTM, though it's not clear the mutation is actually useful.

In principle, it's the right thing to do because it leaves the data in a state that's compatible with previous versions (e.g. v0.14.0, which predates the held write acks change).

In practice it's useless though because downgrading past v0.16.1 doesn't correctly record the schema downgrade so you'll end up corrupting the mutation queue anyway.

It's also not clear it's useful to maintain equivalence with the other platforms either: Web doesn't support downgrades at all and Android has problems because prior to firebase/firebase-android-sdk#149, rerunning migrations would fail.

WDYT?

wilhuff · 2019-01-10T20:47:27Z

Firestore/core/src/firebase/firestore/local/leveldb_migrations.cc

+    Writer writer = Writer::Wrap(&bytes);
+    writer.WriteNanopbMessage(firestore_client_MutationQueue_fields,
+                              &mutation_queue);
+    // transaction.Delete(it->key());


Commented out code?

wilhuff · 2019-01-10T20:52:47Z

Firestore/core/src/firebase/firestore/local/leveldb_migrations.cc

@@ -184,6 +189,39 @@ void RemoveAcknowledgedMutations(leveldb::DB* db) {
  transaction.Commit();
 }

+/** Migration 6. */


This seems the right way to make this work.

This is only really useful in that it leaves the data in a state that's compatible with previous versions that did use this field. This would allow developers to rollback safely to prior versions.

wilhuff · 2019-01-10T20:54:14Z

Firestore/Example/Tests/Local/FSTLevelDBMigrationsTests.mm

+    transaction.Commit();
+  }
+
+  auto get_last_acked = [&] {


This mechanism is not cheaply portable to Android. Consider passing the mutation_queue as a parameter to a regular function.

var-const · 2019-01-10T21:24:10Z

it's not clear the mutation is actually useful.

I guess I'm slightly against it, because value for the user appears pretty small, but touching user data always introduces some room for corruption. So I'd probably omit the migration, but I don't feel strongly about it -- what do you think?

wilhuff · 2019-01-10T21:44:06Z

Based on your comment and the in-person discussion, let's remove it.

var-const · 2019-01-10T22:26:16Z

Removed the migration.

var-const · 2019-01-10T22:27:29Z

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

+ * BatchId values from the local store are non-negative so this value is before
+ * all batches.
+ */
+extern const BatchId kBatchIdUnknown;


Note: I originally only did this to allow leveldb_migrations.cc to remain a pure C++ file, but I guess there's no reason to revert this change.

Now that this is C++ couldn't this just be constexpr?

wilhuff

LGTM with nit.

wilhuff · 2019-01-10T23:23:35Z

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

+ * BatchId values from the local store are non-negative so this value is before
+ * all batches.
+ */
+extern const BatchId kBatchIdUnknown;


Now that this is C++ couldn't this just be constexpr?

wilhuff · 2019-01-10T23:23:48Z

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

@@ -0,0 +1,39 @@
+/*
+ * Copyright 2018 Google


var-const · 2019-01-11T17:44:06Z

Should fix #2237

A straightforward port of firebase/firebase-ios-sdk#2243.

var-const added 4 commits January 9, 2019 17:00

Initial

7dc6324

style.sh

1db0d68

Forgotten

92bc0ab

Fix tests

7f28b14

googlebot added the cla: yes label Jan 9, 2019

var-const commented Jan 9, 2019

View reviewed changes

wilhuff reviewed Jan 9, 2019

View reviewed changes

var-const added 3 commits January 9, 2019 19:13

Initial migration

ff09356

Migration works

28cd407

style.sh

f559b0e

var-const commented Jan 10, 2019

View reviewed changes

var-const assigned wilhuff Jan 10, 2019

var-const added the api: firestore label Jan 10, 2019

Appease linter?

aebc1a6

wilhuff reviewed Jan 10, 2019

View reviewed changes

Leftover code

e35b9ab

var-const force-pushed the varconst/eliminate-highest-ackd-batch-id branch from 3b9570b to e35b9ab Compare January 10, 2019 21:25

Revert the migration

58e4eef

var-const commented Jan 10, 2019

View reviewed changes

wilhuff approved these changes Jan 10, 2019

View reviewed changes

Review feedback

5719e77

var-const merged commit a52abec into master Jan 11, 2019

This was referenced Jan 11, 2019

Crash on reset for nextBatchID by assert in FSTLevelDBMutationQueue.start #2237

Closed

Port "Stop persisting last acknowledged batch ID" from iOS firebase/firebase-js-sdk#1476

Merged

var-const mentioned this pull request Jan 11, 2019

Port "Stop persisting last acknowledged batch ID" from iOS firebase/firebase-android-sdk#188

Merged

var-const added a commit to firebase/firebase-js-sdk that referenced this pull request Jan 14, 2019

Port "Stop persisting last acknowledged batch ID" from iOS

4b71b13

A straightforward port of firebase/firebase-ios-sdk#2243.

paulb777 deleted the varconst/eliminate-highest-ackd-batch-id branch April 13, 2019 16:01

firebase locked and limited conversation to collaborators Oct 23, 2019

Stop persisting last acknowledged batch ID #2243

Stop persisting last acknowledged batch ID #2243

Uh oh!

Conversation

var-const commented Jan 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

var-const commented Jan 10, 2019

Uh oh!

wilhuff commented Jan 10, 2019

Uh oh!

var-const commented Jan 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

var-const commented Jan 11, 2019

Uh oh!

Uh oh!