Refactor pendingWrites / write pipeline. #972

mikelehen · 2018-06-29T21:35:16Z

No functional changes. Just renames, moves, added comments, etc.

pendingWrites => writePipeline
canWriteMutations() => canAddToWritePipeline().
commit() => addToWritePipeline()
lastBatchSeen removed since we can compute it on demand from
writePipeline.
Removed cleanUpWriteStreamState() and instead inlined it in
disableNetworkInternal() since I didn't like the non-symmetry with
cleanUpWatchStreamState() which is called every time the stream closes.
Lots of comment cleanup.

No functional changes. Just renames, moves, added comments, etc. * pendingWrites => writePipeline * canWriteMutations() inverted and renamed isWritePipelineFull(). * commit() => addToWritePipeline() * lastBatchSeen removed since we can compute it on demand from writePipeline. * Removed cleanUpWriteStreamState() and instead inlined it in disableNetworkInternal() since I didn't like the non-symmetry with cleanUpWatchStreamState() which is called every time the stream closes. * Lots of comment cleanup.

schmidt-sebastian

Comments are 10/10. Naming is 9.5/10 :)

I feel that Gil should look at the names of the variables as well (if he has time before his vacation). I am not super sold on isWritePipelineFull. If you have more naming suggestions, please do reach out before submitting. Thanks!

schmidt-sebastian · 2018-06-30T00:35:21Z

packages/firestore/src/remote/remote_store.ts

   */
-  canWriteMutations(): boolean {
+  isWritePipelineFull(): boolean {


The older name did a better job conveying the dual purpose of this function. The new name might create confusion if the function returns true but the pipeline is empty. I'm gonna throw canAcceptMutations out there and see if you like that better.

I reworked into canAddToWritePipeline() for now, which is hopefully abstract enough to encompass isNetworkEnabled, but still concretely ties it to the writePipeline.

I'd actually like to remove the isNetworkEnabled restriction and allow writes in the pipeline when the network is not enabled. But that may be contentious (see Gil's comment on my TODO about that). So I'll propose that in a separate PR and see what we want to do.

wilhuff · 2018-07-01T14:59:25Z

packages/firestore/src/remote/remote_store.ts

-  private pendingWrites: MutationBatch[] = [];
-  private lastBatchSeen: BatchId = BATCHID_UNKNOWN;
+  /**
+   * A set of up to MAX_PENDING_WRITES writes that we have fetched from the


Since order matters, better to call this a "list" of writes.

Not sure, but you could also highlight that the protocol correlates responses with requests implicitly, so it's the order within this list that maintains the correlation.

wilhuff · 2018-07-01T15:05:46Z

packages/firestore/src/remote/remote_store.ts

+   * LocalStore via fillWritePipeline() and have or will send to the write
+   * stream.
+   *
+   * Whenever writePipeline.length > 0 (and the network is enabled) the write


If the network is disabled can the writePipeline have any writes in it? This phrasing makes it seem as if it's possible for there to be writes even when the network is disabled (and suggests the question of how to consider those).

If we can't have writes in here with the network disabled it's probably better to just get that out of the way early.

As written we go out of our way to clear the the pipeline when the network is disabled and avoid adding writes. But I think the code would be simpler (and still work) if we didn't do that. So I'm tempted to change this behavior in a subsequent change (I added a TODO to disableNetworkInternal already).

But for now I've adjusted the comment to match the implemented behavior. Thanks.

wilhuff · 2018-07-01T15:15:04Z

packages/firestore/src/remote/remote_store.ts

+   *
+   * Whenever writePipeline.length > 0 (and the network is enabled) the write
+   * stream is considered to be necessary and all the writes will be sent to the
+   * backend if/when the stream is established.


"if/when" can just be "when".

You may also consider rephrasing this into active voice. For example, rather than "considered to be necessary":

Whenever writePipeline.length > 0 the RemoteStore will attempt to start or restart the write stream. When the stream is established the writes in the pipeline will be sent in order.

Better, thanks!

wilhuff · 2018-07-01T15:19:40Z

packages/firestore/src/remote/remote_store.ts

+          this.writePipeline.length +
+          ' pending writes'
+      );
+      // TODO(mikelehen): We only actually need to clear the write pipeline if


It seems reasonable to me that we only attempt to fill the write pipeline if the network is enabled. I think you can drop this todo.

I'm torn on this.

We're currently going out of our way to avoid filling writePipeline when the network is disabled (I could remove this.writePipeline = [] here and the networkEnabled check in isWritePipelineFull() if we allowed writePipeline to persist when the network was disabled).

It means we do redundant work to re-load the same set of mutations from LocalStore when you re-enable the network.

I think it may be conceptually simpler to just consider writePipeline to be the "pre-fetched" mutations from the LocalStore, regardless of the network state... since they already persist across stream close / restarts.

So I'm inclined to leave it for now.

wilhuff · 2018-07-01T15:20:45Z

packages/firestore/src/remote/remote_store.ts

  /**
-   * Notifies that there are new mutations to process in the queue. This is
-   * typically called by SyncEngine after it has sent mutations to LocalStore.
+   * Attempts to fill our write pipeline with mutations from the LocalStore.


Let's avoid mixing writes and mutations. Since this is a write pipeline we fill it with writes, no?

wilhuff · 2018-07-01T15:25:28Z

packages/firestore/src/remote/remote_store.ts

-        // DOSing ourselves then those limits won't be exceeded here and
-        // we'll continue to make progress.
-        for (const batch of this.pendingWrites) {
+        // Drain the write pipeline now that the stream is established.


"Drain" is incorrect because we're not removing anything. At this point we're merely sending all the queued writes but they don't get removed until there's a response from the server.

In this sense "pending writes" was a better name because it didn't suggest direct sequential processing.

Changed to "Send." I agree "pending writes" was a better name once they're sent. But since they may not be sent, I think it was overall a poor name.

mikelehen

Thanks for the feedback. I tweaked comments and reworked isWritePipelineFull() to canAddToWritePipeline() in response. Take another look if you like.

mikelehen · 2018-07-02T14:52:47Z

packages/firestore/src/remote/remote_store.ts

+   * LocalStore via fillWritePipeline() and have or will send to the write
+   * stream.
+   *
+   * Whenever writePipeline.length > 0 (and the network is enabled) the write


As written we go out of our way to clear the the pipeline when the network is disabled and avoid adding writes. But I think the code would be simpler (and still work) if we didn't do that. So I'm tempted to change this behavior in a subsequent change (I added a TODO to disableNetworkInternal already).

But for now I've adjusted the comment to match the implemented behavior. Thanks.

mikelehen · 2018-07-02T15:02:12Z

packages/firestore/src/remote/remote_store.ts

+   *
+   * Whenever writePipeline.length > 0 (and the network is enabled) the write
+   * stream is considered to be necessary and all the writes will be sent to the
+   * backend if/when the stream is established.


Better, thanks!

mikelehen · 2018-07-02T15:07:16Z

packages/firestore/src/remote/remote_store.ts

+          this.writePipeline.length +
+          ' pending writes'
+      );
+      // TODO(mikelehen): We only actually need to clear the write pipeline if


I'm torn on this.

We're currently going out of our way to avoid filling writePipeline when the network is disabled (I could remove this.writePipeline = [] here and the networkEnabled check in isWritePipelineFull() if we allowed writePipeline to persist when the network was disabled).

It means we do redundant work to re-load the same set of mutations from LocalStore when you re-enable the network.

I think it may be conceptually simpler to just consider writePipeline to be the "pre-fetched" mutations from the LocalStore, regardless of the network state... since they already persist across stream close / restarts.

So I'm inclined to leave it for now.

mikelehen · 2018-07-02T15:08:42Z

packages/firestore/src/remote/remote_store.ts

  /**
-   * Notifies that there are new mutations to process in the queue. This is
-   * typically called by SyncEngine after it has sent mutations to LocalStore.
+   * Attempts to fill our write pipeline with mutations from the LocalStore.


mikelehen · 2018-07-02T15:17:01Z

packages/firestore/src/remote/remote_store.ts

   */
-  canWriteMutations(): boolean {
+  isWritePipelineFull(): boolean {


I reworked into canAddToWritePipeline() for now, which is hopefully abstract enough to encompass isNetworkEnabled, but still concretely ties it to the writePipeline.

I'd actually like to remove the isNetworkEnabled restriction and allow writes in the pipeline when the network is not enabled. But that may be contentious (see Gil's comment on my TODO about that). So I'll propose that in a separate PR and see what we want to do.

mikelehen · 2018-07-02T15:18:25Z

packages/firestore/src/remote/remote_store.ts

-        // DOSing ourselves then those limits won't be exceeded here and
-        // we'll continue to make progress.
-        for (const batch of this.pendingWrites) {
+        // Drain the write pipeline now that the stream is established.


Changed to "Send." I agree "pending writes" was a better name once they're sent. But since they may not be sent, I think it was overall a poor name.

mikelehen · 2018-07-02T15:22:05Z

packages/firestore/src/remote/remote_store.ts

-  private pendingWrites: MutationBatch[] = [];
-  private lastBatchSeen: BatchId = BATCHID_UNKNOWN;
+  /**
+   * A set of up to MAX_PENDING_WRITES writes that we have fetched from the


schmidt-sebastian

I like the new symmetry between canAddToWritePipeline and addToWritePipeline. Thanks for updating the comments!

[Port of firebase/firebase-js-sdk#972] No functional changes. Just renames, moves, added comments, etc. * pendingWrites => writePipeline * canWriteMutations renamed canAddToWritePipeline * commitBatch => addBatchToWritePipeline * lastBatchSeen removed since we can compute it on demand from writePipeline. * Removed cleanUpWriteStreamState and instead inlined it in disableNetworkInternal since I didn't like the non-symmetry with cleanUpWatchStreamState which is called every time the stream closes. * Misc. comment cleanup.

mikelehen assigned schmidt-sebastian Jun 29, 2018

mikelehen requested a review from schmidt-sebastian June 29, 2018 21:35

mikelehen requested a review from wilhuff as a code owner June 29, 2018 21:35

google-oss-bot added the needs-triage label Jun 29, 2018

lint fix

be6733c

schmidt-sebastian approved these changes Jun 30, 2018

View reviewed changes

schmidt-sebastian assigned mikelehen and wilhuff and unassigned schmidt-sebastian Jun 30, 2018

wilhuff reviewed Jul 1, 2018

View reviewed changes

Michael Lehenbauer added 2 commits July 2, 2018 08:35

CR feedback.

6367046

Merge branch 'master' into mikelehen/remove-lastBatchSeen

edf2b70

mikelehen commented Jul 2, 2018

View reviewed changes

schmidt-sebastian approved these changes Jul 2, 2018

View reviewed changes

mikelehen merged commit 63e1414 into master Jul 2, 2018

mikelehen deleted the mikelehen/remove-lastBatchSeen branch July 2, 2018 16:42

firebase locked and limited conversation to collaborators Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor pendingWrites / write pipeline. #972

Refactor pendingWrites / write pipeline. #972

mikelehen commented Jun 29, 2018 •

edited

Loading

schmidt-sebastian left a comment

schmidt-sebastian Jun 30, 2018

mikelehen Jul 2, 2018

wilhuff Jul 1, 2018

mikelehen Jul 2, 2018

wilhuff Jul 1, 2018

mikelehen Jul 2, 2018

wilhuff Jul 1, 2018

mikelehen Jul 2, 2018

wilhuff Jul 1, 2018

mikelehen Jul 2, 2018

wilhuff Jul 1, 2018

mikelehen Jul 2, 2018

wilhuff Jul 1, 2018

mikelehen Jul 2, 2018

mikelehen left a comment

mikelehen Jul 2, 2018

mikelehen Jul 2, 2018

mikelehen Jul 2, 2018

mikelehen Jul 2, 2018

mikelehen Jul 2, 2018

mikelehen Jul 2, 2018

mikelehen Jul 2, 2018

schmidt-sebastian left a comment

Refactor pendingWrites / write pipeline. #972

Refactor pendingWrites / write pipeline. #972

Conversation

mikelehen commented Jun 29, 2018 • edited Loading

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

mikelehen 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

schmidt-sebastian left a comment

Choose a reason for hiding this comment

mikelehen commented Jun 29, 2018 •

edited

Loading