Add a type parameter to Persistence #1047

gsoltis · 2018-07-25T22:14:22Z

Adds a type parameter to Persistence to specify the transaction type. Threads the parameter through where necessary. The end result is being able to remove some casts and adds the ability to add persistence-layer-specific fields to transactions in a type-safe manner.

No behavior changes are intended.

schmidt-sebastian

I have some questions regarding potential changes in the behavior of the tests, but otherwise this looks fine to me.

I do believe that @mikelehen should look at this to see if he agrees with the general direction of this PR.

schmidt-sebastian · 2018-07-26T16:38:37Z

packages/firestore/src/local/memory_persistence.ts

+ * Memory persistence is not actually transactional, but future implementations
+ * may have transaction-scoped state.
+ */
+export class MemoryPersistenceTransaction implements PersistenceTransaction {}


I would rename MemoryPersistenceTransaction to MemoryTransaction, which would match IndexedDbTransaction. Otherwise, maybe MemoryPersistedTransaction?

Done. MemoryTransaction.

schmidt-sebastian · 2018-07-26T16:46:11Z

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

-  });
-
-  genericMutationQueueTests();
+  genericMutationQueueTests(persistenceHelpers.testMemoryPersistence);


We seem to be doing this in local_store.test.ts as well, but this might bite us later as this loses the this context. I would suggest you use an array function here.

I can change it, but persistenceHelpers is a module, not an instance of a class. This is a reference to a function, not a method. For this to bite us, we would need to change the type of persistenceHelpers to something where this would matter. Replacing a module with a class would I think be a significant enough change to warrant auditing the places where it was imported and catch things like this.

schmidt-sebastian · 2018-07-26T16:48:00Z

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

  });

-  genericMutationQueueTests();
+  genericMutationQueueTests(() => persistencePromise);


Is it possible to pass persistence here? Then you don't have to deal with persistencePromise anywhere. Alternatively, you could do:

return persistencePromise.then(p => genericMutationQueueTests(p))

This unfortunately does not work, as it will break if another test uses only. The reason is that describe will always run, creating the persistence layer, but the beforeEach and afterEach methods will not, so it won't be properly cleaned up. The key is to only actually create the persistence layer in a beforeEach method.

Actually, I believe this will break even without another test using only. At the time that you would return this, persistencePromise is undefined. beforeEach needs to run before you can use persistencePromise.

Is this refactoring related to the TransactionType generic? If so, is it obsolete now? It seems fine, but just wanting to make sure it's not just leftover churn.

I think leftover churn. I'm removing it.

schmidt-sebastian · 2018-07-26T16:50:58Z

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

-  });
-
-  genericQueryCacheTests();
+  genericQueryCacheTests(persistenceHelpers.testMemoryPersistence);


Same comment as above regarding the arrow function.

schmidt-sebastian · 2018-07-26T16:52:57Z

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

-  afterEach(() => persistence.shutdown(/* deleteData= */ true));
-
-  genericQueryCacheTests();
+  genericQueryCacheTests(() => persistencePromise);


The previous implementation used a different persistence object for every test case. Can you verify that this is still the case?

Yes, the promise is recreated each time beforeEach runs, so each test run gets its own persistence instance.

mikelehen · 2018-07-26T19:54:37Z

Uhhh..... This seems okay, but honestly, I'm not a huge fan of it. It adds a bit of compile-time safety but at the expense of code churn and making a bunch of our types generic (including LocalStore, LocalDocumentsView, etc. which don't really make sense as generic types and aren't generic on the other platforms).

A better option forward (for simplicity and platform consistency) might be to just drop the explicit transaction passing on web and make the current transaction implicit like it is on Android / iOS. At the time we decided to make it explicit, iOS didn't have transactions and we hadn't tackled Android persistence yet. So I think there's an argument to be made that web could be switched to implicit now. But that's also a large refactoring with lots of code churn and I'm not sure there's anything specific to motivate doing that right now.

That said, I'm not going to object to merging this if the consensus is that it improves the state of affairs. @wilhuff Any interest in weighing in? :)

gsoltis · 2018-07-26T20:24:00Z

As a data point, there will soon be transaction-specific state (sequence numbers). The iOS lru branch has them already, but they are attached to Persistence, because the transaction is implicit. This results in assertions in the persistence layer that you are only accessing this state while in a transaction. The web client is actually better set up to handle this exactly because the transaction is not implicit.

mikelehen · 2018-07-26T20:33:25Z

Fascinating! What is a transaction-specific sequence number? :)

wilhuff · 2018-07-26T20:35:17Z

In the iOS port, the assertion that you're currently in a transaction is made centrally so I don't think there's any cognitive overhead to making the transaction implicit.

Given that we will only ever use one transaction at a time in the local store implicit vs explicit doesn't matter, so all things considered I'd rather change one port to conform to the other two than change two to conform to one. (To be specific, I prefer implicit.)

gsoltis · 2018-07-26T20:38:47Z

We assign the transaction number to QueryData updates, as well as ack'd mutations. This lets us assess how recently used documents are. Here's the assert in FSTLevelDB: https://github.com/firebase/firebase-ios-sdk/blob/lru/Firestore/Source/Local/FSTLevelDB.mm#L118. After much back and forth, @wilhuff and I settled on reaching to the persistence layer and asserting that you're in a transaction as the least bad way of doing this on iOS. Explicit transactions are a more natural place to include this value, as they are valid and constant for the lifetime of a transaction.

My next branch after this one for the webclient adds readonly currentSequenceNumber as a field on PersistenceTransaction.

gsoltis · 2018-07-26T20:41:57Z

I think implicit leads to a lot of hidden gotchas on what methods must be called in a transaction. Having an explicit transaction makes it much clearer. My opinion on this is based on having had to update the test suite to make sure that transactions existed whenever these methods that accessed the implicit transaction were called. It was painful, and in a lot of cases it is not clear why a transaction needs to exist other than if you run it, you will get a runtime error if you don't have one.

wilhuff · 2018-07-26T20:54:06Z

I'm okay with explicit transactions too--I have no real preference. I'm coming at it from the point of conserving work.

gsoltis · 2018-07-30T21:04:33Z

Ok, I think I have addressed everything, including adding a helper. I also left in some of the test refactor, since I think it's a little cleaner this way.

mikelehen

LGTM. Thanks!

mikelehen · 2018-07-31T00:04:41Z

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

    cache = new TestQueryCache(persistence, persistence.getQueryCache());
    await cache.start();
  });

+  afterEach(async () => {
+    persistence.shutdown(/* deletaData= */ true);


Actually, since it looks like you're going to need to merge master and let travis re-run anyway, I'll point out that you missed the second typo in this line. delet_e_Data

This was apparently a load-bearing typo. CI has been failing since I fixed it...

* 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)

* 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)

* 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)

Greg Soltis added 10 commits July 23, 2018 10:58

Cherry pick sequence number starting point

2c93300

Working on typed transactions

96d732c

Start plumbing in sequence number

59121ba

Back out sequence number changes

4b75267

[AUTOMATED]: Prettier Code Styling

29505fc

Fix tests

d03f608

[AUTOMATED]: Prettier Code Styling

e7efc7b

Fix lint

fd1359a

[AUTOMATED]: Prettier Code Styling

9a76422

Merge branch 'master' into typed_transactions

a7b46dd

gsoltis assigned schmidt-sebastian Jul 25, 2018

gsoltis requested a review from schmidt-sebastian July 25, 2018 22:14

gsoltis requested review from mikelehen and wilhuff as code owners July 25, 2018 22:14

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

gsoltis removed the needs-triage label Jul 25, 2018

Greg Soltis added 4 commits July 25, 2018 15:14

Uncomment line

3abdbbe

Fix the merge conflicts

5b66917

MemoryPersistenceTransaction -> MemoryTransaction

a2848ff

[AUTOMATED]: Prettier Code Styling

70e8e51

schmidt-sebastian suggested changes Jul 26, 2018

View reviewed changes

gsoltis assigned mikelehen Jul 26, 2018

mikelehen assigned wilhuff Jul 26, 2018

Greg Soltis added 11 commits July 30, 2018 11:08

Review feedback

d4f1dcc

[AUTOMATED]: Prettier Code Styling

a94c026

Merge branch 'master' into typed_transactions

fa05379

Revert some unintentional import churn

068a4af

Line 44 should definitely be empty

8e46ab9

Checkpoint before adding helper function for stores

091038c

Use a helper for casting PersistenceTransaction to IndexedDbTransaction

f0ed2dd

[AUTOMATED]: Prettier Code Styling

ca53e31

Remove errant generic type

4193323

Lint

1a39850

Merge branch 'master' into typed_transactions

80a2459

gsoltis assigned mikelehen and schmidt-sebastian and unassigned gsoltis Jul 30, 2018

schmidt-sebastian approved these changes Jul 30, 2018

View reviewed changes

mikelehen approved these changes Jul 31, 2018

View reviewed changes

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

mikelehen reviewed Jul 31, 2018

View reviewed changes

Greg Soltis added 2 commits July 31, 2018 09:23

Merge branch 'master' into typed_transactions

7d14f14

Fix typo

9e570c8

gsoltis merged commit 063f729 into master Jul 31, 2018

gsoltis deleted the gsoltis/typed_transactions branch July 31, 2018 17:48

schmidt-sebastian mentioned this pull request Aug 1, 2018

Merge #1073

Merged

firebase locked and limited conversation to collaborators Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a type parameter to Persistence #1047

Add a type parameter to Persistence #1047

gsoltis commented Jul 25, 2018

schmidt-sebastian left a comment

schmidt-sebastian Jul 26, 2018

gsoltis Jul 26, 2018

schmidt-sebastian Jul 26, 2018

gsoltis Jul 26, 2018

schmidt-sebastian Jul 26, 2018

gsoltis Jul 26, 2018

gsoltis Jul 26, 2018

mikelehen Jul 30, 2018

gsoltis Jul 30, 2018

schmidt-sebastian Jul 26, 2018

schmidt-sebastian Jul 26, 2018

gsoltis Jul 26, 2018

mikelehen commented Jul 26, 2018

gsoltis commented Jul 26, 2018

mikelehen commented Jul 26, 2018

wilhuff commented Jul 26, 2018

gsoltis commented Jul 26, 2018

gsoltis commented Jul 26, 2018 •

edited

Loading

wilhuff commented Jul 26, 2018

gsoltis commented Jul 30, 2018

mikelehen left a comment

mikelehen Jul 31, 2018

gsoltis Jul 31, 2018

Add a type parameter to Persistence #1047

Add a type parameter to Persistence #1047

Conversation

gsoltis commented Jul 25, 2018

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

mikelehen commented Jul 26, 2018

gsoltis commented Jul 26, 2018

mikelehen commented Jul 26, 2018

wilhuff commented Jul 26, 2018

gsoltis commented Jul 26, 2018

gsoltis commented Jul 26, 2018 • edited Loading

wilhuff commented Jul 26, 2018

gsoltis commented Jul 30, 2018

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

gsoltis commented Jul 26, 2018 •

edited

Loading