Merging Persistent Stream refactor #1069

schmidt-sebastian · 2018-07-31T17:21:30Z

This "merges" the Persistent Stream refactor (#1041) into the multi-tab branch.

mikelehen · 2018-07-31T23:14:59Z

packages/firestore/src/remote/remote_store.ts

   */
-  private networkAllowed = true;
+  private canUseNetwork = false;


Hrm.... My hope would be that we don't need to track 3 booleans and instead canUseNetwork would be a function of networkEnabled and isPrimary. As-is I'm not sure what their relation is.

We need a boolean to indicate whether the network connection is established, so that we can shut it down when needed in disableNetwork(). At first, this was called networkEnabledInternal, but I then realizes that I need the same state in the canAddToWritePipeline and in the asserts that verify that a stream didn't close unexpectedly.

If I want to use a separate canUseNetwork() method that uses both isPrimary and networkEnabled, then I need to flip networkEnabled to false during shutdown. Otherwise, I am hitting a bunch of asserts in the teardown.

mikelehen · 2018-07-31T23:17:44Z

packages/firestore/test/unit/specs/write_spec.test.ts

@@ -1245,4 +1245,30 @@ describeSpec('Writes:', [], () => {
      );
    }
  );
+
+  specTest(
+    'Mutation are not sent twice after primary failover',


Wow, did you really write a new test to test the merge of this? Or where did this come from?

Yeah, I wanted to verify that we clear the write pipeline during failover.

googlebot · 2018-08-01T17:12:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

schmidt-sebastian · 2018-08-01T17:13:10Z

I merged in your changes. Let me know if the rest of this PR is good to merge.

* Implement global resume token (#1052) * Add a spec test that shows correct global resume token handling * Minimum implementation to handle global resume tokens * Remove unused QueryView.resumeToken * Avoid persisting the resume token unless required * Persist the resume token on unlisten * Add a type parameter to Persistence (#1047) * Cherry pick sequence number starting point * Working on typed transactions * Start plumbing in sequence number * Back out sequence number changes * [AUTOMATED]: Prettier Code Styling * Fix tests * [AUTOMATED]: Prettier Code Styling * Fix lint * [AUTOMATED]: Prettier Code Styling * Uncomment line * MemoryPersistenceTransaction -> MemoryTransaction * [AUTOMATED]: Prettier Code Styling * Review updates * Style * Lint and style * Review feedback * [AUTOMATED]: Prettier Code Styling * Revert some unintentional import churn * Line 44 should definitely be empty * Checkpoint before adding helper function for stores * Use a helper for casting PersistenceTransaction to IndexedDbTransaction * [AUTOMATED]: Prettier Code Styling * Remove errant generic type * Lint * Fix typo * Port optimizations to LocalDocumentsView from iOS (#1055) * add a method to find batches affecting a set of keys (port of [1479](firebase/firebase-ios-sdk#1479)); * use the newly-added method to avoid rereading batches when getting documents in `LocalDocumentsView` (port of [1505](firebase/firebase-ios-sdk#1505)); * avoid rereading batches when searching for documents in a collection (port of [1533](firebase/firebase-ios-sdk#1533)). Speedup was measured by running tests in browser and checking time spent writing 10 batches of 500 mutations each, and then querying the resulting 5K docs collection from cache in offline mode. For this case, the writing speedup is about 3x, and querying speedup is about 6x (see PR for more details). * Add a CHANGELOG entry for #1052 (#1071) * Add a CHANGELOG entry for #1052 * Add notes for #1055 * Rename idleTimer and fix comments. (#1068) * Merge (#1073)

* Catch invalid provider id error (#1064) * RxFire: Api Change and documentation (#1066) * api changes and doc updates * fixes * Refactor PersistentStream (no behavior changes). (#1041) This breaks out a number of changes I made as prep for b/80402781 (Continue retrying streams for 1 minute (idle delay)). PersistentStream changes: * Rather than providing a stream event listener to every call of start(), the stream listener is now provided once to the constructor and cannot be changed. * Streams can now be restarted indefinitely, even after a call to stop(). * PersistentStreamState.Stopped was removed and we just return to 'Initial' after a stop() call. * Added `closeCount` member to PersistentStream in order to avoid bleedthrough issues with auth and stream events once stop() has been called. * Calling stop() now triggers the onClose() event listener, which simplifies stream cleanup. * PersistentStreamState.Auth renamed to 'Starting' to better reflect that it encompasses both authentication and opening the stream. RemoteStore changes: * Creates streams once and just stop() / start()s them as necessary, never recreating them completely. * Added networkEnabled flag to track whether the network is enabled or not, since we no longer null out the streams. * Refactored disableNetwork() / enableNetwork() to remove stream re-creation. Misc: * Comment improvements including a state diagram on PersistentStream. * Fixed spec test shutdown to schedule via the AsyncQueue to fix sequencing order I ran into. * Merging Persistent Stream refactor (#1069) * Merging PersistentStream refactor * [AUTOMATED]: Prettier Code Styling * Typo * Remove canUseNetwork state. (#1076) * Merging the latest merge into the previous merge (#1077) * Implement global resume token (#1052) * Add a spec test that shows correct global resume token handling * Minimum implementation to handle global resume tokens * Remove unused QueryView.resumeToken * Avoid persisting the resume token unless required * Persist the resume token on unlisten * Add a type parameter to Persistence (#1047) * Cherry pick sequence number starting point * Working on typed transactions * Start plumbing in sequence number * Back out sequence number changes * [AUTOMATED]: Prettier Code Styling * Fix tests * [AUTOMATED]: Prettier Code Styling * Fix lint * [AUTOMATED]: Prettier Code Styling * Uncomment line * MemoryPersistenceTransaction -> MemoryTransaction * [AUTOMATED]: Prettier Code Styling * Review updates * Style * Lint and style * Review feedback * [AUTOMATED]: Prettier Code Styling * Revert some unintentional import churn * Line 44 should definitely be empty * Checkpoint before adding helper function for stores * Use a helper for casting PersistenceTransaction to IndexedDbTransaction * [AUTOMATED]: Prettier Code Styling * Remove errant generic type * Lint * Fix typo * Port optimizations to LocalDocumentsView from iOS (#1055) * add a method to find batches affecting a set of keys (port of [1479](firebase/firebase-ios-sdk#1479)); * use the newly-added method to avoid rereading batches when getting documents in `LocalDocumentsView` (port of [1505](firebase/firebase-ios-sdk#1505)); * avoid rereading batches when searching for documents in a collection (port of [1533](firebase/firebase-ios-sdk#1533)). Speedup was measured by running tests in browser and checking time spent writing 10 batches of 500 mutations each, and then querying the resulting 5K docs collection from cache in offline mode. For this case, the writing speedup is about 3x, and querying speedup is about 6x (see PR for more details). * Add a CHANGELOG entry for #1052 (#1071) * Add a CHANGELOG entry for #1052 * Add notes for #1055 * Rename idleTimer and fix comments. (#1068) * Merge (#1073)

schmidt-sebastian added 2 commits July 31, 2018 10:19

Merging PersistentStream refactor

b86b756

[AUTOMATED]: Prettier Code Styling

dd280c6

schmidt-sebastian requested review from mikelehen and wilhuff as code owners July 31, 2018 17:21

google-oss-bot added the needs-triage label Jul 31, 2018

Typo

8556bcc

schmidt-sebastian assigned mikelehen Jul 31, 2018

mikelehen suggested changes Jul 31, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jul 31, 2018

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Aug 1, 2018

Remove canUseNetwork state. (#1076)

38e46a4

mikelehen approved these changes Aug 1, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Aug 1, 2018

schmidt-sebastian merged commit ee3bec1 into mulittab-streammerge-dirty Aug 1, 2018

schmidt-sebastian deleted the multitab-streammerge branch August 3, 2018 17:15

firebase locked and limited conversation to collaborators Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merging Persistent Stream refactor #1069

Merging Persistent Stream refactor #1069

schmidt-sebastian commented Jul 31, 2018

mikelehen Jul 31, 2018 •

edited

Loading

schmidt-sebastian Aug 1, 2018

mikelehen Jul 31, 2018

schmidt-sebastian Aug 1, 2018

googlebot commented Aug 1, 2018

schmidt-sebastian commented Aug 1, 2018

Merging Persistent Stream refactor #1069

Merging Persistent Stream refactor #1069

Conversation

schmidt-sebastian commented Jul 31, 2018

mikelehen Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

schmidt-sebastian Aug 1, 2018

Choose a reason for hiding this comment

mikelehen Jul 31, 2018

Choose a reason for hiding this comment

schmidt-sebastian Aug 1, 2018

Choose a reason for hiding this comment

googlebot commented Aug 1, 2018

schmidt-sebastian commented Aug 1, 2018

mikelehen Jul 31, 2018 •

edited

Loading