Fix latency-compensated query missing patch mutations. #621
Conversation
ghost
added
the
ghost
requested a review
from 
| @@ -182,6 +184,14 @@ private MaybeDocument getDocument(DocumentKey key, List<MutationBatch> inBatches | |||
| remoteDocumentCache.getAllDocumentsMatchingQuery(query); | |||
|
|
|||
| List<MutationBatch> matchingBatches = mutationQueue.getAllMutationBatchesAffectingQuery(query); | |||
| // It is possible that a PatchMutation can make a document match a query, even if | |||
There was a problem hiding this comment.
This is a useful comment but it interrupts the flow of this otherwise large method. Consider moving this to be a doc comment on your new method (which doesn't otherwise have comments).
| @@ -210,4 +215,33 @@ private MaybeDocument getDocument(DocumentKey key, List<MutationBatch> inBatches | |||
|
|
|||
| return results; | |||
| } | |||
|
|
|||
| /** | |||
| * It is possible that a PatchMutation can make a document match a query, even if the version in | |||
There was a problem hiding this comment.
Ultranit: some class/function names are backtick-escaped, and some aren't. Consider doing backticks for consistency (PatchMutation).
There was a problem hiding this comment.
Please don't backtick in javadoc. There's no markdown support. Use <code>/</code>.
There was a problem hiding this comment.
Thanks. Looking at other comments in the file, they use {@code foo}.
|
|
||
| /** | ||
| * It is possible that a PatchMutation can make a document match a query, even if the version in | ||
| * the RemoteDocumentCache is not a match yet (waiting for server to ack). To handle this, we find |
There was a problem hiding this comment.
Ultranit: backtick RemoteDocumentCache.
| /** | ||
| * It is possible that a PatchMutation can make a document match a query, even if the version in | ||
| * the RemoteDocumentCache is not a match yet (waiting for server to ack). To handle this, we find | ||
| * all document keys affected by the PatchMutations that are not in `result` yet, and back fill |
There was a problem hiding this comment.
Ultranit: backtick PatchMutation.
| * the RemoteDocumentCache is not a match yet (waiting for server to ack). To handle this, we find | ||
| * all document keys affected by the PatchMutations that are not in `result` yet, and back fill | ||
| * them via `remoteDocumentCache.getAll`, otherwise those `PatchMutations` will be ignored because | ||
| * no base document can be found, and lead to missing result for the query. |
There was a problem hiding this comment.
Ultranit: s/result/results
| */ | ||
| private ImmutableSortedMap<DocumentKey, Document> addMissingBaseDocuments( | ||
| List<MutationBatch> matchingBatches, ImmutableSortedMap<DocumentKey, Document> existingDocs) { | ||
| HashSet<DocumentKey> missedDocKeys = new HashSet<>(); |
There was a problem hiding this comment.
Optional: s/missedDocKeys/missingDocKeys.
| } | ||
|
|
||
| ImmutableSortedMap<DocumentKey, Document> mergedDocs = existingDocs; | ||
| Map<DocumentKey, MaybeDocument> missedDoc = remoteDocumentCache.getAll(missedDocKeys); |
There was a problem hiding this comment.
Nit: s/missedDoc/missingDocs
| } | ||
|
|
||
| ImmutableSortedMap<DocumentKey, Document> mergedDocs = existingDocs; | ||
| Map<DocumentKey, MaybeDocument> missingDoc = remoteDocumentCache.getAll(missingDocKeys); |
There was a problem hiding this comment.
Sorry to nitpick, but it should be missingDocs.
No description provided.