Make getNewDocumentChanges() idempotent #2255
Conversation
| value.readTime | ||
| ); | ||
| // PORTING NOTE: This is only used for multi-tab synchronization. | ||
| getLastDocumentChange( |
There was a problem hiding this comment.
This is used in the synchronizeLastProcessedReadTime version of LocalStore. The changedDoc is never read, but I opted to return it to make the API similar to getDocumentChanges above.
| @@ -192,6 +200,11 @@ export class LocalStore { | |||
| this.queryEngine.setLocalDocumentsView(this.localDocuments); | |||
| } | |||
|
|
|||
| /** Starts the LocalStore. */ | |||
| start(): Promise<void> { | |||
| return this.synchronizeLastDocumentChangeReadTime(); | |||
There was a problem hiding this comment.
This is equivalent to the previous functionality in RemoteDocumentStore.start().
| assertMatches([], changedDocs); | ||
| }); | ||
|
|
||
| it('can get changes', async () => { |
There was a problem hiding this comment.
These tests moved so they are only exercised against IndexedDb.
| @@ -168,6 +169,13 @@ export class LocalStore { | |||
| q.canonicalId() | |||
| ); | |||
|
|
|||
| /** | |||
| * The read time of the last entry processed by `getNewDocumentChanges()`. | |||
There was a problem hiding this comment.
getDocumentChanges?
Oh! I see: this refers to getNewDocumentChanges in LocalStore and not the one in the RemoteDocumentCache. This is slightly confusing and I have a slight preference for keeping the names the same, though up to you.
There was a problem hiding this comment.
I see your confusion:) I tried to be to smart about it and reverted back to using getNewDocumentChanges() everywhere, which still makes sense even when a read time is passed (at least I see it that way now).
| * time. | ||
| */ | ||
| // PORTING NOTE: This is only used for multi-tab synchronization. | ||
| getDocumentChanges( |
There was a problem hiding this comment.
FWIW, the getNewDocumentChanges still makes sense while explicitly passing the readTime. That name helps convey that it's a diff of changes.
I don't oppose renaming it to getDocumentChanges if that's your preference, but I don't think it needed to change.
| * the point of rejection. | ||
| */ | ||
| // PORTING NOTE: This is only used for multi-tab synchronization. | ||
| getNewDocumentChanges( |
There was a problem hiding this comment.
An alternative to making LocalStore aware that it's using IndexedDB persistence would be to leave this method in the interface and allow persistence implementations to throw some kind of unsupported operation error if they're ever called.
This would have the effect of making the LocalStore code cleaner. Both would fail if MemoryPersistence was ever used with multi-tab. As is we fail the assertion; the proposed change would throw unsupported operation.
There was a problem hiding this comment.
I added the method back to the interface and am throwing an exception now. I debated between the two approaches, but it probably makes sense to reduce this clutter in LocalStore.
I did leave getLastDocumentChange() IndexdedDb only. This is a new method and there are no asserts for this in LocalStore (just an if-statement that makes the intent clear).
schmidt-sebastian
changed the base branch from
mrschmidt/moresimplecalls
to
mrschmidt/idempotent
* Add transaction retries (#2250) * Marking SimpleDb calls as idempotent (#2251) * Mark mostly readonly calls as idempotent (#2252) * Fix test failure (#2256) * Make handleUserChange idempotent (#2257) * Temporarily disable CountingQueryEngine tests (#2258) * Improve test hack (#2259) * Improve test hack * Comment in test hack * Make getNewDocumentChanges() idempotent (#2255) * Add onCommitted listeners for transactions (#2265) * Fix build * Fix Lint * Make applyRemoteEvent idempotent (#2263) * Make notifyLocalViewChanges idempotent (#2268) * Make releaseQuery idempotent (#2266) * Mark acknowledgeBatch and rejectBatch idempotent (#2269) * idempotent `allocateQuery` and `notifyLocalViewChanges` (#2264) * Mark collectGarbage idempotent (#2267) * Idempotency: Address TODOs, add Changelog (#2270)
Ugh. I believe this is one of the more difficult "idempotent" migration since getNewDocumentChanges() changes state in-memory state in the Persistence layer, whereas most other transactions just change in-memory state in LocalStore.
To make
getNewDocumentChanges()idempotent, I move the change watermark into LocalStore, which then lets me set it only after the transaction is committed. Instead of making this work for MemoryRemoteDocumentStore, I opted to just assert thatgetNewDocumentChanges()is never called (it is only used for multi-tab).