Remove Held Write Acks #1156

schmidt-sebastian · 2018-08-22T01:10:45Z

This PR removes held write acks. Instead, we store the mutation result directly in IndexedDb as the Write stream acknowledges a mutation. These documents are marked with hasCommittedMutations.

For documents with hasCommittedMutations which are part of the initial View (modified before the query was added), we don't raise hasPendingWrites. Only mutations that are acknowledged during the lifetime of a query raise hasPendingWrites. This is meant to mimic the previous behavior, since we did not hold writes when we did not have queries.

When we don't have a base document, patches that can't be applied are stored as UnknownDocuments until Watch catches up.

Existing held write acks are dropped via schema migration. This PR removes all other write ack handling.

wilhuff · 2018-08-24T20:19:06Z

packages/firestore/src/model/document.ts

-  constructor(readonly key: DocumentKey, readonly version: SnapshotVersion) {}
+  constructor(
+    readonly key: DocumentKey,
+    readonly remoteVersion: SnapshotVersion


What's a "remoteVersion"? Both the watch-stream-assigned read_time and the write-stream-assigned commit_version are "remote" in nature. I don't think this name helps disambiguate (if it's intended to).

The Firestore RPC API exposes the following versions:
https://github.com/googleapis/googleapis/blob/468e07f7a9eaa3ecb4fac719bbd28745a5485cc4/google/firestore/v1beta1/firestore.proto#L39

It seems to me that we should not introduce new names with new meanings unless we're specifically diverging from these names. If we are diverging we should add fairly extensive comments about how this is supposed to fit together.

Based on your changes to the serializer, this seems like mostly the update_time but sometimes the read_time.

Would it make sense to remove remoteVersion here and instead keep updatedVersion in Document and readVersion in NoDocument?

For documents, we need to be able to tell the commitTime apart from udpateTime. Currently, one version is stored inside api.Document and serialized as part of the document, while the other is serialized in a separate field.

We can just store a single version and add dirty boolean. We could then go back and name the one version that we have version and make it the readTime for NoDocument and the last modified time for Documents (which is either the commitTime or the updateTime).

What do you think?

FYI, I pushed a commit that replaces the two different versions with a "dirty" boolean that I named hasCommittedMutations.

schmidt-sebastian · 2018-08-28T17:19:33Z

PR updated with the result of our discussion:

We store a "dirty" bit in IndexedDb.
We store "unknown documents" in IndexedDb. These are documents that are known to exist at a specific version, but have no data. This allows us to simply some of our View processing logic.
We store deleted documents at the time of their delete.

wilhuff

Mostly LGTM

wilhuff · 2018-08-28T21:56:38Z

packages/firestore/src/local/indexeddb_schema.ts

@@ -425,6 +425,14 @@ export class DbNoDocument {
  constructor(public path: string[], public readTime: DbTimestamp) {}
 }

+/**
+ * Represents a document that is know to exist but whose data is unknwon.


s/know to exist/known to exist/
s/unknwon/unknown/

wilhuff · 2018-08-28T21:57:21Z

packages/firestore/src/local/indexeddb_schema.ts

@@ -437,6 +445,12 @@ export class DbRemoteDocument {
  static store = 'remoteDocuments';

  constructor(
+    /**
+     * Set to an instance of DbUnknownDocument if the data for a document is
+     * not known, but it is known that a document exist at the specified version


s/exist at/exists at/

wilhuff · 2018-08-28T22:03:22Z

packages/firestore/src/local/indexeddb_schema.ts

+export class DbUnknownDocument {
+  constructor(public path: string[], public readTime: DbTimestamp) {}
+}
+
 /**
 * An object to be stored in the 'remoteDocuments' store in IndexedDb. It
 * represents either a cached document (if it exists) or a cached "no-document"


This comment could do with updating. Something like:

An object to be stored in the 'remoteDocuments' store in IndexedDb. It represents one of three things:

A complete document

A "no document" representing a document that is known not to exist (at some version)

An "unknown document" representing a document that is known to exist (at some version) but whose contents are unknown.

wilhuff · 2018-08-28T22:07:34Z

packages/firestore/src/local/indexeddb_schema.ts

+     * documents are potentially inconsistent with the document's version on
+     * the backend.
+     */
+    public hasCommittedMutations: boolean | undefined


We keep calling this a "dirty" bit when discussing it and I don't think this name is that much clearer for all its extra verbosity.

The comment above could also indicate that this has an impact on how to interpret the version stored inside the different document types.

I updated the commit.

I opted to stick with hasCommittedMutations for now. It is the same name that I use in Document, and I chose that name for two reasons:

It works well in combination with hasLocalMutations and explains the either-or state that these two states convey. Only one of hasLocalMutations or hasCommittedMutations can be true at any given time.

Similarly, calling this dirty poses the question of whether documents with hasLocalMutations are dirty. If the answer to that is a resounding yes, that would mean we have to update all tests that use hasLocalMutations to also set dirty.

I can see that. Agreed.

wilhuff · 2018-08-28T22:11:15Z

packages/firestore/src/model/document.ts

@@ -110,9 +127,39 @@ export class NoDocument extends MaybeDocument {
    return `NoDocument(${this.key}, ${this.version})`;
  }

-  isEqual(other: NoDocument): boolean {
+  hasPendingWrites(): boolean {
+    return false;


If we have a committed delete with an acknowledged version greater than the watch snapshot version, wouldn't we want that to be true here?

If not, could you add comments here about why this is necessarily false?

In an ideal world, we would want this to be true. We however don't support hasPendingWrites on deletes (In master, we only ever used Document.hasLocalMutations). If we want to change this behavior, then I would like to do this later.

Comment added.

wilhuff · 2018-08-28T22:24:35Z

packages/firestore/src/model/mutation.ts

@@ -220,7 +226,7 @@ export abstract class Mutation {
  abstract applyToRemoteDocument(
    maybeDoc: MaybeDocument | null,
    mutationResult: MutationResult
-  ): MaybeDocument | null;
+  ): MaybeDocument;


The comment above indicates that this can still return null.

I updated this comment as well as the broader class comment.

wilhuff · 2018-08-28T22:30:03Z

packages/firestore/src/model/mutation.ts

@@ -377,13 +383,12 @@ export class PatchMutation extends Mutation {
    // patch, so we use the precondition to prevent incorrectly putting a
    // partial document into our cache.
    if (!this.precondition.isValidFor(maybeDoc)) {
-      return maybeDoc;
+      return new UnknownDocument(this.key, mutationResult.version);


This doesn't seem right, or at least not intuitively so, based on how we've documented this type. This needs a comment that describes why this is an unknown document. I thought we should preserve null until we discussed offline, so maybe something like:

If we're here applying a mutation to a remote document, the server must not have rejected the write, the precondition must have held, so the document must actually exist.

Added the following comment:

// Since the mutation was not rejected, we know that the precondition // matched on the backend. We therefore must not have the expected version // of the document in our cache and return an UnknownDocument with the // known updateTime.

wilhuff · 2018-08-28T22:55:08Z

packages/firestore/src/model/mutation.ts

@@ -377,13 +383,12 @@ export class PatchMutation extends Mutation {
    // patch, so we use the precondition to prevent incorrectly putting a


You can axe this TODO here. This change pretty much addresses it.

wilhuff · 2018-08-28T23:04:48Z

packages/firestore/src/model/mutation.ts

@@ -484,17 +489,19 @@ export class TransformMutation extends Mutation {
    // patch, so we use the precondition to prevent incorrectly putting a
    // partial document into our cache.
    if (!this.precondition.isValidFor(maybeDoc)) {
-      return maybeDoc;
+      return new UnknownDocument(this.key, mutationResult.version);


Same comments as above (axe the TODO and describe why this document must actually exist).

wilhuff · 2018-08-28T23:08:06Z

packages/firestore/src/remote/datastore.ts

-      return this.serializer.fromWriteResults(response.writeResults);
+      return this.serializer.fromWriteResults(
+        response.writeResults,
+        response.commitTime


Does this need a bang for strict null checks? (i.e. response.commitTime!)

I ask because the assignment to const commitVersion uses one below.

fromWriteResults takes an optional commit time and changes it to a non-optional commitTime. Empty WriteBatches return no commit time, and hence we have to check that we have at least one write response before we can use the commit time.

schmidt-sebastian

Comments addressed

schmidt-sebastian · 2018-08-29T00:20:06Z

packages/firestore/src/local/indexeddb_schema.ts

@@ -425,6 +425,14 @@ export class DbNoDocument {
  constructor(public path: string[], public readTime: DbTimestamp) {}
 }

+/**
+ * Represents a document that is know to exist but whose data is unknwon.


schmidt-sebastian · 2018-08-29T00:20:12Z

packages/firestore/src/local/indexeddb_schema.ts

@@ -437,6 +445,12 @@ export class DbRemoteDocument {
  static store = 'remoteDocuments';

  constructor(
+    /**
+     * Set to an instance of DbUnknownDocument if the data for a document is
+     * not known, but it is known that a document exist at the specified version


schmidt-sebastian · 2018-08-29T00:25:27Z

packages/firestore/src/local/indexeddb_schema.ts

+     * documents are potentially inconsistent with the document's version on
+     * the backend.
+     */
+    public hasCommittedMutations: boolean | undefined


I updated the commit.

I opted to stick with hasCommittedMutations for now. It is the same name that I use in Document, and I chose that name for two reasons:

It works well in combination with hasLocalMutations and explains the either-or state that these two states convey. Only one of hasLocalMutations or hasCommittedMutations can be true at any given time.

Similarly, calling this dirty poses the question of whether documents with hasLocalMutations are dirty. If the answer to that is a resounding yes, that would mean we have to update all tests that use hasLocalMutations to also set dirty.

schmidt-sebastian · 2018-08-29T00:30:49Z

packages/firestore/src/model/document.ts

@@ -110,9 +127,39 @@ export class NoDocument extends MaybeDocument {
    return `NoDocument(${this.key}, ${this.version})`;
  }

-  isEqual(other: NoDocument): boolean {
+  hasPendingWrites(): boolean {
+    return false;


In an ideal world, we would want this to be true. We however don't support hasPendingWrites on deletes (In master, we only ever used Document.hasLocalMutations). If we want to change this behavior, then I would like to do this later.

Comment added.

schmidt-sebastian · 2018-08-29T00:35:56Z

packages/firestore/src/model/mutation.ts

@@ -79,9 +84,10 @@ export class FieldTransform {
 export class MutationResult {
  constructor(
    /**
-     * The version at which the mutation was committed or null for a delete.
+     * The version at which the mutation was committed, either extracted from


Updated the comment to use your suggested phrasing.

schmidt-sebastian · 2018-08-29T00:44:14Z

packages/firestore/src/model/mutation.ts

@@ -220,7 +226,7 @@ export abstract class Mutation {
  abstract applyToRemoteDocument(
    maybeDoc: MaybeDocument | null,
    mutationResult: MutationResult
-  ): MaybeDocument | null;
+  ): MaybeDocument;


I updated this comment as well as the broader class comment.

schmidt-sebastian · 2018-08-29T00:51:38Z

packages/firestore/src/model/mutation.ts

@@ -377,13 +383,12 @@ export class PatchMutation extends Mutation {
    // patch, so we use the precondition to prevent incorrectly putting a
    // partial document into our cache.
    if (!this.precondition.isValidFor(maybeDoc)) {
-      return maybeDoc;
+      return new UnknownDocument(this.key, mutationResult.version);


Added the following comment:

// Since the mutation was not rejected, we know that the precondition // matched on the backend. We therefore must not have the expected version // of the document in our cache and return an UnknownDocument with the // known updateTime.

schmidt-sebastian · 2018-08-29T00:52:07Z

packages/firestore/src/model/mutation.ts

@@ -484,17 +489,19 @@ export class TransformMutation extends Mutation {
    // patch, so we use the precondition to prevent incorrectly putting a
    // partial document into our cache.
    if (!this.precondition.isValidFor(maybeDoc)) {
-      return maybeDoc;
+      return new UnknownDocument(this.key, mutationResult.version);


schmidt-sebastian · 2018-08-29T00:53:02Z

packages/firestore/src/remote/datastore.ts

-      return this.serializer.fromWriteResults(response.writeResults);
+      return this.serializer.fromWriteResults(
+        response.writeResults,
+        response.commitTime


fromWriteResults takes an optional commit time and changes it to a non-optional commitTime. Empty WriteBatches return no commit time, and hence we have to check that we have at least one write response before we can use the commit time.

wilhuff

LGTM, except I'm not sure this can go into master as is.

Specifically:

Don't we need a migration to convert acknowledged mutations still in the mutation queue to documents with hasCommittedChanges true?
Don't we need to change some of the code that handles NoDocument specially? For example: Precondition.exists needs to account for this new kind of document.

Happy to take these as follow-on PRs

wilhuff · 2018-08-29T01:29:29Z

packages/firestore/src/local/indexeddb_schema.ts

+     * documents are potentially inconsistent with the document's version on
+     * the backend.
+     */
+    public hasCommittedMutations: boolean | undefined


I can see that. Agreed.

wilhuff · 2018-08-29T01:33:38Z

packages/firestore/src/model/mutation.ts

+ * have no explicit update time, we use the commitTime of the WriteResponse for
+ * Delete mutations.
+ *
+ * If due to precondition mismatches an acknowledged mutation can't be applied


This language is hard to understand. How about:

If a mutation is acknowledged but locally fails its precondition, we return an UnknownDocument and rely on Watch to send us the updated version.

Simplified to:

* If a mutation is acknowledged by the backend but fails the precondition check * locally, we return an `UnknownDocument` and rely on Watch to send us the * updated version.

wilhuff · 2018-08-29T01:35:47Z

packages/firestore/test/unit/local/local_store.test.ts

@@ -315,8 +312,12 @@ function genericLocalStoreTests(
      )
      .toContain(doc('foo/bar', 0, { foo: 'bar' }, { hasLocalMutations: true }))
      .afterAcknowledgingMutation({ documentVersion: 1 })
-      .toReturnChanged(doc('foo/bar', 0, { foo: 'bar' }))
-      .toContain(doc('foo/bar', 0, { foo: 'bar' }))
+      .toReturnChanged(


wilhuff · 2018-08-29T01:37:52Z

packages/firestore/test/unit/model/mutation.test.ts

-    assertVersionTransitions(set, docV3, mutationResult, docV3);
-    assertVersionTransitions(set, deletedV3, mutationResult, docV0);
-    assertVersionTransitions(set, null, mutationResult, docV0);
+    assertVersionTransitions(set, docV3, mutationResult, docV7Committed);


This test essentially implements the version transitions in that big comment on top of Mutation. Could you update the comment? Alternatively, if you didn't find the table helpful, maybe we we should just delete it?

The test invokes applyToRemoteDocument, which has changed a lot and hence the changes to the test. The table in the comment is for applyToLocalView, which should still be the same.

schmidt-sebastian · 2018-08-29T16:11:46Z

Specifically:

Don't we need a migration to convert acknowledged mutations still in the mutation queue to documents with hasCommittedChanges true?

I don't think so. They will get replayed as before and create Documents with hasLocalMutatations. With this PR, we now have two codepaths that do the same thing.

Don't we need to change some of the code that handles NoDocument specially? For example: Precondition.exists needs to account for this new kind of document.

I will look at that.

In general, I wasn't going to merge this PR until I know that these changes work with my main held-write-acks-removal branch.

* Checking for UnknownDocument * Remove unused import * Adding an assert in Transaction

schmidt-sebastian requested review from mikelehen and wilhuff as code owners August 22, 2018 01:10

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

Adding CommitVersion to RemoteDocumentCache

9a981b1

schmidt-sebastian force-pushed the mrschmidt-commitversion branch from 05fe956 to 9a981b1 Compare August 22, 2018 01:14

schmidt-sebastian assigned wilhuff Aug 22, 2018

Fix random test problem

fe82b46

wilhuff reviewed Aug 24, 2018

View reviewed changes

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

wilhuff added api: firestore and removed needs-triage labels Aug 24, 2018

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

schmidt-sebastian added 6 commits August 24, 2018 17:09

Merge

cd3095d

Adding a dirty bit

c1c3063

Merge UnknownDocument

4993479

Adding authoritative updates back

37ecd7a

Comment fixes

af30849

[AUTOMATED]: Prettier Code Styling

1e15aa1

Lint fixes

aa853da

wilhuff reviewed Aug 28, 2018

View reviewed changes

Comments

2e5a8b3

schmidt-sebastian requested review from gsoltis, rsgowman, var-const and zxu123 as code owners August 29, 2018 01:04

schmidt-sebastian commented Aug 29, 2018

View reviewed changes

wilhuff approved these changes Aug 29, 2018

View reviewed changes

Comment update

411c4bd

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

schmidt-sebastian added 5 commits August 30, 2018 13:49

Merge branch 'master' into mrschmidt-commitversion

92ae0fc

Checking for UnknownDocument (#1174)

8b903fe

* Checking for UnknownDocument * Remove unused import * Adding an assert in Transaction

Rename UnknownDocument.readTime to version (#1193)

4672c3d

Comment cleanup (#1196)

e6c2222

Remove Held Write Acks (#1135)

71d9fbc

schmidt-sebastian changed the title ~~Adding CommitVersion to RemoteDocumentCache~~ Remove Held Write Acks Sep 5, 2018

schmidt-sebastian merged commit ad2d178 into master Sep 5, 2018

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

firebase locked and limited conversation to collaborators Oct 16, 2019

		@@ -377,13 +383,12 @@ export class PatchMutation extends Mutation {
		// patch, so we use the precondition to prevent incorrectly putting a

Remove Held Write Acks #1156

Remove Held Write Acks #1156

Uh oh!

Conversation

schmidt-sebastian commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

schmidt-sebastian commented Aug 28, 2018

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!

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!

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!

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!

schmidt-sebastian 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!

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!

schmidt-sebastian commented Aug 22, 2018 •

edited

Loading