Implement sequence number migration #1374
Conversation
gsoltis
requested review from
mikelehen,
rsgowman,
var-const,
wilhuff and
zxu123
as code owners
| ); | ||
|
|
||
| return getHighestListenSequenceNumber(txn).next(currentSequenceNumber => { | ||
| const writeSentinelKey = ( |
There was a problem hiding this comment.
I think this would be easier to read if a) this was declared one level higher (as a direct child of ensureSequenceNumbers) and b) if you used a normal JS function (function writeSentinelKey() {}).
Optional.
There was a problem hiding this comment.
Leaving as-is.
It can't go in a higher scope as it closes over the result of getHighestListenSequenceNumber(). And typescript disallows nested function declarations, so it needs to be a lambda. I'm guessing this is because function and lambdas have different bindings for this. Nested functions make it easy to make a mistake when calling methods off of this.
| .iterate((key, doc) => { | ||
| const path = new ResourcePath(key); | ||
| const docSentinelKey = sentinelKey(path); | ||
| documentTargetStore.get(docSentinelKey).next(maybeSentinel => { |
There was a problem hiding this comment.
You need to push this promise (the one returned from documentTargetStore.get) rather then the result of writeSentinelKey. With the current code, you may miss some writes if the reads don't come back fast enough.
There was a problem hiding this comment.
Good catch. I restructured it a little to remove the inner call to push() and instead push every read. Reads which require an additional write return a persistence promise in the next() call.
Port of firebase/firebase-android-sdk#64
Ensures that every document in the cache has a corresponding sentinel row with a sequence number. Precursor to enabling LRU in production.