Adding SnapshotVersion to SharedClientState #1151
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt-sharedsnapshotversion
branch
from
878c14b to
d7d23d6
Compare
schmidt-sebastian
force-pushed
the
mrschmidt-sharedsnapshotversion
branch
from
d7d23d6 to
982d65d
Compare
| @@ -90,6 +91,7 @@ class NoOpSharedClientStateSyncer implements SharedClientStateSyncer { | |||
| constructor(private readonly activeClients: ClientId[]) {} | |||
| async applyBatchState( | |||
| batchId: BatchId, | |||
| snapshotVersion:SnapshotVersion, | |||
There was a problem hiding this comment.
Missing space after colon.
| @@ -759,6 +783,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer { | |||
|
|
|||
| private async emitNewSnapsAndNotifyLocalStore( | |||
| changes: MaybeDocumentMap, | |||
| snapshotVersion: SnapshotVersion, | |||
There was a problem hiding this comment.
What is this snapshot version? A read version? A commit version? The highest of either? What happens if a caller passes SnapshotVersion.MIN?
Looking at callers I can't tell if they're passing the right values.
Assuming that passing the commit version and the read version is legit here, should we name this highestVersion? highWatermarkVersion?
Basically, I think naming these new variables and parameters "snapshotVersion" isn't helping: these should always be named after the concrete kind of version they are.
There was a problem hiding this comment.
It's the version that the backend associates with the given operation - either the global snapshot version or the commit version of the write. I am not sure if the name can be much more specific :/
I did however go back and changed how this method gets its argument. I am using something similar to what I do in multi-tab (applying a "fake" current change via a synthesized RemoteEvent) and have added a helper that creates a "remote event" for an acknowledged write. This remote event is then used to propagate the commit version.
This gets rid of the argument and of some of the ambiguity. What do you think?
schmidt-sebastian
force-pushed
the
mrschmidt-sharedsnapshotversion
branch
from
a9d7b77 to
58849ed
Compare
schmidt-sebastian
force-pushed
the
mrschmidt-sharedsnapshotversion
branch
from
58849ed to
c8178fe
Compare
| * Create a synthesized remote event that is used to apply the commit version | ||
| * of a successful write to a View. | ||
| */ | ||
| static createSynthesizedRemoteEventForSuccessfulWrite( |
There was a problem hiding this comment.
I'm probably missing something here, but I think this is a mistake. It's conflating commit versions (that only come from write acknowledgements) with the update/read versions that come from watch snapshots. I don't think we ever want those values to end up in the same place, so faking a remote event to tunnel in a commit version seems like the wrong way to go.
Ignoring the plumbing for a bit, it seems like the semantics we should have locally should start by tracking the semantics from the server. This suggests the following scheme:
- Documents have an update time reflecting the last update via watch. Documents that have never been seen via watch would not have an update time.
- Documents have a commit time reflecting the last commit time via write acknowledgement. Documents coming from watch have no commit time (they only have an update time).
- Deleted documents mirror this scheme, except they don't have an update time--they have a read time (that seems functionally equivalent). It's an open question as to whether or not this needs to be reflected in our models. We could certainly track them separately and implement a watchVersion that exposes either value based on the type. I don't think this detail matters for the purposes of this discussion though, so let's ignore deletes for the moment.
So, to implement the per-document gate that prevents watch from creating write-synchronization flicker with an older version that our current commit, we check:
- If the document has no commit version, we haven't written it recently, so just allow the update through
- Otherwise, if the update version is less than the commit version, drop the update (watch is behind)
- If the update version is greater than or equal to the commit version, allow the update through, and the document will once again have an update version but no commit version.
In this way, presence of a commit version marks the document as dirty.
The reverse comparison also has to be made, just in case the write acknowledgement comes after the watch update. Namely if a document comes through with a commit version that's less than or equal to the update version we discard the change that results from the acknowledgement.
These changes seem like they're localized to LocalStore.acknowledgeBatch and LocalStore.applyRemoteEvent. The MaybeDocumentMap containing the changes should end up with the versions correctly annotated (per document).
What falls out of this is that by the time we load the documents all the documents should already have the commit versions set so there's no additional synthesis required.
I think this means that we don't need to plumb snapshot versions into applyBatchState/applyTargetState etc, because the documents themselves will carry the values necessary.
I'm not clear on whether or not this means we can further avoid the need to plumb the snapshot version into the QueryTargetMetadata or the MutationBatchMetadata. I don't entirely understand how these are used.
There was a problem hiding this comment.
The plumbing in this PR is mostly used to update last snapshot version processed by the Views, which in turn is used to disregard updates for documents that we tried to patch locally but don't have a version for. The version will be passed to computeDocChanges and is not necessarily part of the maybeDocumentMap (https://github.com/firebase/firebase-js-sdk/pull/1135/files#diff-59002bb6a4e41ae4ef48ff413744e9d3R134). It's even somewhat likely that the maybeDocumentMap is empty but the snapshotVersion is set.
I am not opposed to only ever storing one version - readTime, updateTime or commitTime. I never need more than one. But I need a way to plumb the updateTime of a document that I don't have the data for to the Views. And on top of that, I need to be able to plumb this version through LocalStorage.
|
This PR is no longer needed, since we now store UnknownDocuments in persistence. |
This PR is a part of #1135 and adds the SnapshotVersion to the state that is tracked in LocalStorage. This allows us to use this snapshot version in secondary clients during multi-tab to update their views.
A change from #1135 is that I updated the code to store the timestamps as an object of { seconds, nanos} instead of plain microseconds.