Remove Held Write Acks #1135

schmidt-sebastian · 2018-08-18T01:07:38Z

This PR removes held write acks, while mostly maintaining the existing user-facing properties. Instead of keeping mutations in the mutation queue, they are directly applied to the RemoteDocumentCache. As part of this, the cache entry now tracks the "commit version" and we can thus tell whether a document is dirty.

If a document is mutated after query initialization time (the first SnapshotVersion received from the backend), we raise hasPendingWrites. For documents that were modified before the query was registered, we raise isFromCache (this is meant to mirror the previous behavior, as we cleared write acks for no listen).

There is special logic to that deals with patches to unknown documents. Updates to these documents get filtered in the view until Watch catches up.

This is a large PR that rewrites some of our core view handling, as as such, it is likely beneficial to split this up. I am sending this out in one big chunk for now to get some high level feedback. Once that has been addressed, I plan to split this PR up.

Possible splitting points:

Extract the schema migration that drops held write acks. This will also include the plumbing of DatabaseId to createOrUpgradeDb.
Changing of removeMutationBatches to only accept a single batch and dropping of getHighestAcknowledgedBatchID.
Addition of SnapshotVersion to SharedClientState (used to update the Views of secondary tabs)
The rest of the PR, including the new and updated Spec tests.

There are also some more unit tests that I should add for the View changes.

wilhuff · 2018-08-20T17:37:56Z

packages/firestore/src/core/view.ts

+   * of this view. Any document edited after will have `hasPendingWrites` set
+   * until Watch catches up.
+   */
+  private firstSnapshotVersion = SnapshotVersion.MIN;


This comment doesn't make sense to me. "first snapshot version that watch has sent" seems like an immutable property, but then "any document edited after will have hasPendingWrites" suggests that either we'll never clear hasPendingWrites or this value will actually change as Watch gives us updates.

Also "edited" is imprecise, are you referring to our having something in the mutation queue or are you referring to our receiving a write acknowledgement?

Finally, this pair of names suggest a range but I don't understand what the range actually means and the comments here don't specify how to treat these as a range. Perhaps suggesting these are related in this way isn't helpful?

I removed these snapshot versions:

Since we store UnknownDocuments in the database, we no longer need to check whether the snapshots "went back in time".

I added a special computeInitialChanges() method that gets rid of the need for firstSnapshotVersion. This method differs from computeDocChanges in that ignores hasCommittedMutations for the first view snapshot.

wilhuff · 2018-08-20T17:42:07Z

packages/firestore/src/core/view.ts

+   * ignore events if the Write and the Watch stream send us update out of
+   * order.
+   */
+  private lastSnapshotVersion = SnapshotVersion.MIN;


"view has been synced with" is imprecise: is this a watch snapshot version, a write acknowledgement version?

Basically: the protocol specifies versions in various places but I don't understand how those versions map onto these versions. I think this would be clearer if the definitions of these values were less squishy and more directly corresponded to values we already understand and then use derived properties to drive the logic.

For example, If I'm understanding correctly, I think this is describing the maximum of the latest watch snapshot version and the write acknowledgement version. Could we just store watchSnapshotVersion and commitVersion, and define a highestVersion that returns the max of them?

This comment should have been resolved by the changes in this and the earlier PRs.

wilhuff · 2018-08-20T17:47:37Z

packages/firestore/src/core/view.ts

@@ -105,12 +125,15 @@ export class View {
   * need to go back to the local cache for more results. Does not make any
   * changes to the view.
   * @param docChanges The doc changes to apply to this view.
+   * @param snapshotVersion The snapshot version that triggered the doc changes


The comments suggest that firstSnapshotVersion is the first version from watch, but nothing about this signature requires that to be the case. Is there any way to plumb this such that this version only comes from a remote event (thus guaranteeing that it actually comes from watch)?

This argument has been removed.

wilhuff · 2018-08-20T17:58:11Z

packages/firestore/src/local/local_store.ts

            // If a document update isn't authoritative, make sure we don't
            // apply an old document version to the remote cache. We make an
            // exception for SnapshotVersion.MIN which can happen for
            // manufactured events (e.g. in the case of a limbo document
            // resolution failing).
+
+            if (authoritativeUpdates.has(doc.key)) {


This seems to have rephrased the previous version without any real change (i.e. what was wrong with all of these phrases being ||ed together?

Secondarily this seems complicated enough that we should pull it out into a separate method. The caller is getting big enough that we should push heavily to move complicated code out to keep this readable.

Finally, these conditions seem to overlap in ways that could make the cases easier to understand if we combined these into larger branches.

I wanted to be able to flip shouldApplyUpdate back to false, and that's how I ended up changing these statements.

In the end, most of these changes are no longer needed so I reverted almost all of this.

wilhuff · 2018-08-20T18:18:17Z

packages/firestore/src/local/memory_remote_document_cache.ts

-          new NoDocument(key, SnapshotVersion.forDeletedDoc())
+          new NoDocument(
+            key,
+            SnapshotVersion.forDeletedDoc(),


This seems confusing that we need two versions here and that they would use two different names for the same value.

All documents now only have one version.

wilhuff · 2018-08-20T18:20:15Z

packages/firestore/src/local/simple_db.ts

    name: string,
    version: number,
    runUpgrade: (
      db: IDBDatabase,
+      databaseId: DatabaseId,


This seems unused?

This was used in the serializer. Addressed in earlier PR.

wilhuff · 2018-08-20T18:24:01Z

packages/firestore/src/model/document.ts

@@ -89,21 +109,26 @@ export class Document {
 * denotes time we know it didn't exist at.
 */
 export class NoDocument {
-  constructor(readonly key: DocumentKey, readonly version: SnapshotVersion) {}
+  constructor(


These versions aren't really used in here. Also there's no hasPendingWrites defined for this type, which seems confusing.

Is it time to make MaybeDocument a base class rather than a type union?

This has been resolved in an earlier PR.

wilhuff · 2018-08-20T18:27:43Z

packages/firestore/src/model/document.ts

@@ -51,19 +52,38 @@ export class Document {
    return this.data.value();
  }

+  hasPendingWrites(baseSnapshotVersion: SnapshotVersion): boolean {


Needs comments.

I don't intrinsically understand how "has pending writes" relates to a snapshot version. Is the snapshot version a version from watch? A commit version? The name "baseSnapshotVersion" doesn't help me understand this either. Is the base version coming from watch? From storage?

Also, while this definitely operates on properties of a document it seems like this notion of hasPendingWrites is something that is much more closely related to the algorithms of whether or not we accept an update, and would likely be better off kept with that code. As it stands, that logic is now sort of weirdly split up, and it's not like documents intrinsically have this property in any way that could be expressed universally.

This is much simpler now: A Document has pending writes when either localMutations or committedMutations are present. A NoDocument has pending writes when committedMutations (note that this is internal only and not exposed to the user).

wilhuff · 2018-08-20T18:32:16Z

packages/firestore/src/model/document.ts

+    // If the document was committed after Watch has delivered the base
+    // snapshot, we raise `hasPendingWrites` as long as the local commit version
+    // remains higher than the version sent to us by Watch.
+    if (


As written this seems like it could just be:

return !baseSnapshotVersion.isEqual(SnapshotVersion.MIN) && this.commitVersion.compareTo(baseSnapshotVersion) >= 0 && this.commitVersion.compareTo(this.remoteVersion) > 0;

However, I don't understand why the commitVersion > remoteVersion has anything to do with baseSnapshotVersion. If the commit version is greater than the remote version, why does the baseSnapshotVersion matter at all?

No longer applicable.

schmidt-sebastian · 2018-08-20T19:38:53Z

packages/firestore/src/local/shared_client_state.ts

@@ -173,6 +176,7 @@ export interface SharedClientState {
 * encoded as part of the key.
 */
 interface MutationMetadataSchema {
+  snapshotVersionMicros: number;


We should store this as seconds/nanos.

No longer applicable since we don't need to look these version up from LocalStorage.

schmidt-sebastian

Addressed all comments and resolved all issues that were raised within the last two weeks. Back to you, Gil.

schmidt-sebastian · 2018-08-30T21:28:35Z

packages/firestore/src/core/view.ts

+   * of this view. Any document edited after will have `hasPendingWrites` set
+   * until Watch catches up.
+   */
+  private firstSnapshotVersion = SnapshotVersion.MIN;


I removed these snapshot versions:

Since we store UnknownDocuments in the database, we no longer need to check whether the snapshots "went back in time".

I added a special computeInitialChanges() method that gets rid of the need for firstSnapshotVersion. This method differs from computeDocChanges in that ignores hasCommittedMutations for the first view snapshot.

schmidt-sebastian · 2018-08-30T21:29:01Z

packages/firestore/src/core/view.ts

+   * ignore events if the Write and the Watch stream send us update out of
+   * order.
+   */
+  private lastSnapshotVersion = SnapshotVersion.MIN;


This comment should have been resolved by the changes in this and the earlier PRs.

schmidt-sebastian · 2018-08-30T21:29:14Z

packages/firestore/src/core/view.ts

@@ -105,12 +125,15 @@ export class View {
   * need to go back to the local cache for more results. Does not make any
   * changes to the view.
   * @param docChanges The doc changes to apply to this view.
+   * @param snapshotVersion The snapshot version that triggered the doc changes


This argument has been removed.

schmidt-sebastian · 2018-08-30T21:31:03Z

packages/firestore/src/local/local_store.ts

            // If a document update isn't authoritative, make sure we don't
            // apply an old document version to the remote cache. We make an
            // exception for SnapshotVersion.MIN which can happen for
            // manufactured events (e.g. in the case of a limbo document
            // resolution failing).
+
+            if (authoritativeUpdates.has(doc.key)) {


I wanted to be able to flip shouldApplyUpdate back to false, and that's how I ended up changing these statements.

In the end, most of these changes are no longer needed so I reverted almost all of this.

schmidt-sebastian · 2018-08-30T21:31:25Z

packages/firestore/src/local/memory_remote_document_cache.ts

-          new NoDocument(key, SnapshotVersion.forDeletedDoc())
+          new NoDocument(
+            key,
+            SnapshotVersion.forDeletedDoc(),


All documents now only have one version.

schmidt-sebastian · 2018-08-30T21:31:56Z

packages/firestore/src/local/shared_client_state.ts

@@ -173,6 +176,7 @@ export interface SharedClientState {
 * encoded as part of the key.
 */
 interface MutationMetadataSchema {
+  snapshotVersionMicros: number;


No longer applicable since we don't need to look these version up from LocalStorage.

schmidt-sebastian · 2018-08-30T21:47:49Z

packages/firestore/src/local/simple_db.ts

    name: string,
    version: number,
    runUpgrade: (
      db: IDBDatabase,
+      databaseId: DatabaseId,


This was used in the serializer. Addressed in earlier PR.

schmidt-sebastian · 2018-08-30T21:49:08Z

packages/firestore/src/model/document.ts

@@ -51,19 +52,38 @@ export class Document {
    return this.data.value();
  }

+  hasPendingWrites(baseSnapshotVersion: SnapshotVersion): boolean {


This is much simpler now: A Document has pending writes when either localMutations or committedMutations are present. A NoDocument has pending writes when committedMutations (note that this is internal only and not exposed to the user).

schmidt-sebastian · 2018-08-30T21:49:21Z

packages/firestore/src/model/document.ts

+    // If the document was committed after Watch has delivered the base
+    // snapshot, we raise `hasPendingWrites` as long as the local commit version
+    // remains higher than the version sent to us by Watch.
+    if (


No longer applicable.

schmidt-sebastian · 2018-08-30T21:49:43Z

packages/firestore/src/model/document.ts

@@ -89,21 +109,26 @@ export class Document {
 * denotes time we know it didn't exist at.
 */
 export class NoDocument {
-  constructor(readonly key: DocumentKey, readonly version: SnapshotVersion) {}
+  constructor(


This has been resolved in an earlier PR.

wilhuff · 2018-09-04T22:09:34Z

packages/firestore/src/core/transaction.ts

@@ -36,7 +37,7 @@ export class Transaction {

  constructor(private datastore: Datastore) {}

-  private recordVersion(doc: MaybeDocument): void {
+  private recordVersion(doc: NoDocument | Document): void {


This construct is non-portable. Might as well take MaybeDocument and assert this is not an UnknownDocument (which is a portable construct).

I already changed this when I did the initial Android port. This function takes a MaybeDocument and has an assert inside of it. The same is now true for the Web.

wilhuff · 2018-09-04T22:11:26Z

packages/firestore/test/util/api_helpers.ts

@@ -87,7 +95,7 @@ export function query(path: string): Query {
 *     document, with the key being the document id, and the value being the document contents.
 * @param docsToAdd Specifies data to be added into the query snapshot as of now. Each entry maps
 *     to a document, with the key being the document id, and the value being the document contents.
- * @param hasPendingWrites Whether the query snapshot has pending writes.
+ * @param mutatedKes The list of document with pending writes.


mutatedKeys?

wilhuff · 2018-09-04T22:15:19Z

packages/firestore/src/core/view.ts

+   * @return A new set of docs, changes, and refill flag.
+   */
+  computeInitialChanges(docs: MaybeDocumentMap): ViewDocumentChanges {
+    // TODO: Call this directly from the constructor and use


Use TODO(name):, though doing this kind of work in the constructor seems like a bad idea, so perhaps just strike the TODO?

Removed the TODO for now. The initial setup to get both a ViewSnapshot and a View involves a lot of back-and-forth of the same data, and I was leaning towards reducing this. I'll think some more about this after all these PRs are merged.

wilhuff · 2018-09-04T22:38:55Z

packages/firestore/src/core/view.ts

-              // That means there may be some doc in the local cache that's
-              // actually less than this one.
-              needsRefill = true;
+              if (


Is this functional change intended? Previously we'd check the limit case in both the full and metadata-only cases but now only in the former case.

It's simpler now to do this check only in this branch (since we otherwise also have to include changeApplied in the if-check). I can't think of a case where a document would fall outside the limit based on a metadata-only change.

wilhuff · 2018-09-04T22:46:03Z

packages/firestore/src/core/view.ts

+    // instead of three (one with `hasPendingWrites`, the modified
+    // document with `hasPendingWrites` and the final state of the
+    // document).
+    return (


This doesn't appear to actually wait for a version (just the local-mutations/committed-mutations properties). That suggests that either this name is wrong (since it suggests a version comparison) or this is missing something critical.

Incidentally where is the version check?

I used to have a version check in here when I still dealt with versions in the view. Now that I have the UnknownDocument, I no longer need to look at the document version, but only on the committed flag. If I know that a document had mutations committed, then I want to wait for the acknowledgement from Watch.

BTW, this essentially ignores all documents updated in a WriteAck and we could look at handling this outside the View.

I renamed this method to shouldWaitForSyncedDocument. I debated naming 'synced' 'remote' but that term is a little bit overloaded already in our SDK.

wilhuff · 2018-09-04T22:53:55Z

packages/firestore/src/local/local_serializer.ts

@@ -56,7 +56,9 @@ export class LocalSerializer {
    } else if (remoteDoc.noDocument) {
      const key = DocumentKey.fromSegments(remoteDoc.noDocument.path);
      const version = this.fromDbTimestamp(remoteDoc.noDocument.readTime);
-      return new NoDocument(key, version);
+      return new NoDocument(key, version, {


nit: it's a bit jarring to see NoDocument taking a keyword argument but DbRemoteDocument with a DbNoDocument taking regular positional parameters. Could this be done consistently?

DbRemoteDocument is the persisted version of the document. I felt that it was saner to directly encode the boolean flag there, especially since the schema is already in JSON and has the name encoded.

DbNoDocument looks this way to match DbDocument.

wilhuff · 2018-09-04T23:00:12Z

packages/firestore/test/unit/api/database.test.ts

@@ -80,32 +80,39 @@ describe('Query', () => {
 describe('QuerySnapshot', () => {
  it('support equality checking with isEqual()', () => {
    expectEqual(
-      querySnapshot('foo', {}, { a: { a: 1 } }, true, false, false),
-      querySnapshot('foo', {}, { a: { a: 1 } }, true, false, false)
+      querySnapshot('foo', {}, { a: { a: 1 } }, keys('coll/a'), false, false),


"coll/a" seems to be unrelated to the collection in question.

Also, it's weird that this can have mutated keys but neither from cache nor sync state changed flags set.

I changed these tests to use foo.

I also went through and removed the mutations from most of these comparisons. Only the one case that directly compared the mutated keys still has mutated keys with both flags false (to make it obvious what is tested here).
BTW, I think this case could happen, albeit it would be rare (the backend has sent us all committed documents, but we are still not CURRENT since there are more edits from other clients).

I also fixed the ViewSnapshot comparison to take mutatedKeys into account.

wilhuff

LGTM

schmidt-sebastian requested review from davideast, mikelehen and wilhuff as code owners August 18, 2018 01:07

google-oss-bot added the needs-triage label Aug 18, 2018

schmidt-sebastian force-pushed the mrschmidt-writeacks-v3 branch 3 times, most recently from b4f0e23 to 89133d2 Compare August 18, 2018 01:15

schmidt-sebastian assigned wilhuff Aug 18, 2018

schmidt-sebastian force-pushed the mrschmidt-writeacks-v3 branch from 89133d2 to c6688ca Compare August 18, 2018 01:25

schmidt-sebastian added api: firestore and removed needs-triage labels Aug 18, 2018

schmidt-sebastian force-pushed the mrschmidt-writeacks-v3 branch 2 times, most recently from 7476bea to 8ecf392 Compare August 18, 2018 07:19

schmidt-sebastian unassigned wilhuff Aug 18, 2018

schmidt-sebastian force-pushed the mrschmidt-writeacks-v3 branch 4 times, most recently from f463a20 to 9962948 Compare August 20, 2018 16:53

schmidt-sebastian assigned wilhuff Aug 20, 2018

Remove Held Write Acks

306ef4c

schmidt-sebastian force-pushed the mrschmidt-writeacks-v3 branch from 6a60efb to 306ef4c Compare August 20, 2018 16:55

wilhuff reviewed Aug 20, 2018

View reviewed changes

schmidt-sebastian commented Aug 20, 2018

View reviewed changes

schmidt-sebastian mentioned this pull request Aug 20, 2018

Change removeMutationBatch to remove a single batch #1148

Merged

Master merge

a6983a2

This was referenced Aug 20, 2018

Schema migration that drops held write acks #1149

Merged

Adding SnapshotVersion to SharedClientState #1151

Closed

schmidt-sebastian added 3 commits August 21, 2018 10:50

Merge MaybeDocument

5c2152c

Merge

528b49d

Merge

0a37d8c

schmidt-sebastian added 2 commits August 30, 2018 13:50

Apply commit version changes

b77266f

Merge

c54a809

schmidt-sebastian requested review from gsoltis, rsgowman, var-const and zxu123 as code owners August 30, 2018 20:53

schmidt-sebastian changed the base branch from master to mrschmidt-commitversion August 30, 2018 21:05

schmidt-sebastian force-pushed the mrschmidt-writeacks-v3 branch from 7993da4 to 67cef8d Compare August 30, 2018 21:53

schmidt-sebastian commented Aug 30, 2018

View reviewed changes

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Aug 30, 2018

Comment changes

72a12e7

schmidt-sebastian force-pushed the mrschmidt-writeacks-v3 branch from 67cef8d to 72a12e7 Compare August 30, 2018 22:07

schmidt-sebastian added 6 commits August 30, 2018 16:20

Test fix

b25edcb

Adding an assert in Transaction

5d1f5ff

Merge branch 'mrschmidt-unknowndocument' into mrschmidt-writeacks-v3

a27b6e8

Use the hasPendingWrites state from the View

05cf0a4

Use the hasPendingWrites state from the View

5b3adba

Merge

9fd05e6

wilhuff reviewed Sep 4, 2018

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Sep 5, 2018

Review feedback

439ac7f

schmidt-sebastian removed their assignment Sep 5, 2018

wilhuff approved these changes Sep 5, 2018

View reviewed changes

schmidt-sebastian added 2 commits September 4, 2018 22:56

Merge

a474724

Test fix

2416dc7

schmidt-sebastian merged commit 71d9fbc into mrschmidt-commitversion Sep 5, 2018

schmidt-sebastian deleted the mrschmidt-writeacks-v3 branch September 5, 2018 18:22

firebase locked and limited conversation to collaborators Oct 17, 2019

Remove Held Write Acks #1135

Remove Held Write Acks #1135

Conversation

schmidt-sebastian commented Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

schmidt-sebastian commented Aug 18, 2018 •

edited

Loading