Add test for getHighestListenSequenceNumber() recovery #3083
Conversation
Binary Size ReportAffected SDKs
Test Logs
|
schmidt-sebastian
force-pushed
the
mrschmidt/failenablepersistence
branch
from
1ba0ce0 to
3efc875
Compare
schmidt-sebastian
requested review from
avolkovi,
davideast,
Feiyang1,
hiranya911,
hsubox76,
jamesdaniels,
sam-gc,
scottcrossen,
yuchenshi and
zwu52
as code owners
schmidt-sebastian
force-pushed
the
mrschmidt/failenablepersistence
branch
from
3efc875 to
12ff025
Compare
schmidt-sebastian
changed the title
| [DbTargetGlobal.store], | ||
| txn => getHighestListenSequenceNumber(txn) | ||
| txn => | ||
| getHighestListenSequenceNumber( |
There was a problem hiding this comment.
There's a getHighestSequenceNumber that does this translation for you, though I'm really dubious that we should really have names that differ in this way for the difference in transaction types.
It seems like there's a change warranted where we minimize the use of SimpleDbTransaction?
There was a problem hiding this comment.
The original version of this PR changed getHighestListenSequenceNumber to take an IndedexDbTransaction. I had to duplicate a couple lines of code in the SimpleDb schema conversion to make this work, since the schema conversion needs the highest sequence number but doesn't use IndedexDbTransactions.
The other callsite that does this conversion is part the TargetCache, which hasn't been started at this point.
I have no preference either way. The cleanest solution was probably the one that changed all sites to use IndedexDbTransaction and added three lines of duplicate code to the schema conversion.
There was a problem hiding this comment.
It's a shame that we're ending up with two different transaction mechanisms and that migrations need to be written differently than the rest of the code. Is that the end state?
Is your intent to make everything mostly use IndedexDbTransaction except for the few places that can't? If so I'm not as concerned about the individual steps along the way.
There was a problem hiding this comment.
I cleaned this up a little, and hope it is an improvement.
- I inlined two of the helper methods and changed them to only use IndedexDbTransactions/PersitenceTransactions.
- I changed the called in IndexedDbPersistence to use the method from the TargetCache. The TargetCache doesn't have a
start(), so there is actually no issue here. - The SimpleDbSchema class now has a couple lines of code duplication.
| const writeSentinelKey = ( | ||
| path: ResourcePath | ||
| ): PersistencePromise<void> => { | ||
| return documentTargetStore.put( | ||
| new DbTargetDocument( | ||
| 0, | ||
| encodeResourcePath(path), | ||
| currentSequenceNumber | ||
| metadata!.highestListenSequenceNumber! |
There was a problem hiding this comment.
We set the metadata earlier in the migration. It should exist at this point.
There was a problem hiding this comment.
I'm wary of this style of commenting (review-only). The reviewer of this PR isn't the only one who's ever going to read this code.
Don't get me wrong--there's definitely a place for this kind of thing, but this comment in particular seems reasonable to just make in the code itself.
There was a problem hiding this comment.
Added a debug assert that contains this message.
| const writeSentinelKey = ( | ||
| path: ResourcePath | ||
| ): PersistencePromise<void> => { | ||
| return documentTargetStore.put( | ||
| new DbTargetDocument( | ||
| 0, | ||
| encodeResourcePath(path), | ||
| currentSequenceNumber | ||
| metadata!.highestListenSequenceNumber! |
There was a problem hiding this comment.
I'm wary of this style of commenting (review-only). The reviewer of this PR isn't the only one who's ever going to read this code.
Don't get me wrong--there's definitely a place for this kind of thing, but this comment in particular seems reasonable to just make in the code itself.
This adds a test that verify that we can bubble up the exception from
getHighestListenSequenceNumbertoenablePersistence(). The client can then proceed with MemoryPersistence:firebase-js-sdk/packages/firestore/src/core/firestore_client.ts
Line 325 in 3de5763
Addresses #2755