STITCH-1960 Conform to synchronization algorithm in specification #70
Conversation
jsflax
left a comment
jsflax
left a comment
There was a problem hiding this comment.
Looks good! This really cleans up the R2L/L2R logic.
Few nits (mostly language related), and a possible minor related to clearing events once "processing" them.
| logicalT, | ||
| eventEntry.getValue().getOperationType())); | ||
|
|
||
| // i. Find the corresponding local document. |
There was a problem hiding this comment.
[nit] i. Find the corresponding local document config. Orthologically and semantically, these are different concepts.
There was a problem hiding this comment.
Sounds good, but this means changing the spec as well since I copied directly from there.
| } | ||
|
|
||
| // TODO(QUESTION FOR REVIEW): This code is not in the spec, but if I don't include it, then | ||
| // testInsertThenSyncUpdateThenUpdate will fail. Should I add it to the spec? |
There was a problem hiding this comment.
In my opinion, it would be helpful to add to the spec. This is more than just an implementation detail.
There was a problem hiding this comment.
It's this in spec:
drop the event if the version counter of the remote event less than or equal to the version counter of the local document
There was a problem hiding this comment.
But that's in a different place. The way the spec is written, this check is only made "if the document does not have local writes pending". Does this mean we should reorder the spec so that the check occurs before the "If the document does not have local writes pending" check?
There was a problem hiding this comment.
I suppose it should be changed in the spec. We do this check if we have changes or not.
| docConfig.getDocumentId(), | ||
| remoteChangeEvent.getOperationType().toString())); | ||
| break; | ||
| return; |
There was a problem hiding this comment.
Nice, I caught this in my current ticket as well.
|
|
||
| // 1. If both the local document version and the remote change event version are empty, drop | ||
| // the event. The absence of a version is effectively a version, and the pending write will | ||
| // set a version on the next L2R pass if it’s not a delete. |
There was a problem hiding this comment.
[nit] Might be nice to add the relative bits of this to the hasVersion doc string.
| continue; | ||
| } | ||
|
|
||
| // i. Retrieve the synthetic change event for this local document in the local config |
There was a problem hiding this comment.
[nit] We may want to be careful about using the word synthetic here. Local events exist in a transient state, but are arguably real. Synthetic as term makes more sense when we are generating remote events based on conflict, and do not have actual data about what the remote change event was. I feel it's important that we do not conflate terminology here.
There was a problem hiding this comment.
Alright, removing the word synthetic, and updating the spec as well.
| this.emitError(docConfig, String.format( | ||
| // ii. Check if the internal remote change stream listener has an unprocessed event for | ||
| // this document. | ||
| final ChangeEvent<BsonDocument> unprocessedRemoteEvent = |
| * | ||
| * @return the latest unprocessed change event for the given document ID, or null if none exists. | ||
| */ | ||
| public @Nullable ChangeEvent<BsonDocument> getUnprocessedEventForDocumentId( |
There was a problem hiding this comment.
[minor?] Once this event is read, shouldn't it be considered processed? That was the logic behind clearing the map in getEvents– once they've been read, they have been processed. It seems like we should be removing them here.
There was a problem hiding this comment.
Agreed with Jason. This isn't a bug but it would cause us to keep doing work that's not needed.
There was a problem hiding this comment.
Ah, good catch, took me longer than it should have to wrap my head around this. I thought that because this is L2R we're not actually processing the event, and that we should wait for R2L to actually process it, but you're right that in any case, the event here is being effectively dropped or used to raise a conflict, so I'll fix this and clarify the comments here.
There was a problem hiding this comment.
Clarified the comments, and added a suggestion to the spec as well.
edaniels
left a comment
edaniels
left a comment
There was a problem hiding this comment.
Great work. LGTM after Jason's comments are resolved.
4 participants
I did this by basically adding the spec to the code as comments. there were several places where I had to add missing functionality, and there’s at least one place where I’m not sure if we need to add more to the spec to make it complete.
I didn’t add any tests since I figured existing tests cover this, and some of the previously unimplemented features of the sync algorithm are more for optimization than correctness.