Don't persist manufactured documents #695
Conversation
| // If a document update isn't authoritative, make sure we don't apply an old document | ||
| // version to the remote cache. We make an exception for SnapshotVersion.MIN which can | ||
| // happen for manufactured events (e.g. in the case of a limbo document resolution | ||
| // failing). |
There was a problem hiding this comment.
As mentioned on chat, this comment is now misleading. It says we don't write old document versions to the remote cache except we make an exception for SnapshotVersion.MIN (implying we do write it to cache). With your change, this is no longer true.
Perhaps remove "We make an exception ..." from this comment and add a new comment inside your new else-if case that says something like:
This is a manufactured event (since version==SnapshotVersion.NONE), likely as a result of a failed limbo resolution lookup. Rather than write the NoDocument to cache with an invalid version, we just remove whatever might currently be in cache.
There was a problem hiding this comment.
I updated it just as you wrote this comment:) Let me know if the new version is a better description. Thanks!
| || (authoritativeUpdates.contains(doc.getKey()) && !existingDoc.hasPendingWrites()) | ||
| || doc.getVersion().compareTo(existingDoc.getVersion()) >= 0) { | ||
| remoteDocuments.add(doc); | ||
| changedDocs.put(key, doc); | ||
| // NoDocuments with SnapshotVersion.MIN are used in manufactured events (e.g. in the |
There was a problem hiding this comment.
This structure make the comment look as if it applies to the block about authoritative updates. Better to put this in the next block (and pull the comment above into this one--above the actions taken).
|
/test device-check-changed |
This PR makes sure we don't write documents to the RemoteDocumentCache that have a version of 0. This allows us to add an assert in the Index-Free query branch and also makes sure that we don't just persist NoDocuments because of RPC failures.