Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle superseded messages properly for incremental mode in GetThreadNonblock CORE-8523 #13249

Merged
merged 7 commits into from
Aug 10, 2018

Conversation

mmaxim
Copy link
Contributor

@mmaxim mmaxim commented Aug 10, 2018

There is a problem in the INCREMENTAL mode of GetThreadNonblock where it does not include messages in the remote result that were modified (by a superseder) from the cached result. This affects edits and deletes of messages that we have cached. Patch does the following to fix the situation:

1.) Change mergeLocalRemoteThread to include messages from the remote result that have a SupersededBy value that is different than what we sent up for the cached result.
2.) Fix how resolveHoles works in HybridConversationSource. Previously, it would try to modify the thread that we fetched from storage in-place. However, it didn't do anything for superseded messages, and so the results we got back from a "holey" cache hit were wrong. Now instead of modifying the thread in resolveHoles, we just re-fetch from Storage if we get a holey hit and resolve it.
3.) Add a new mode for basicSupersedesTransform to include a "hidden" PLACEHOLDER message instead of just dropping the message for deleted messages. This allows us to communicate to clients of GetThreadNonblock that cached messages have actually been deleted.
4.) Bonus Fix: PullLocalOnly had previously been using a SimpleResultCollector regardless of the query passed in. This made it such that it returned a subset of what Pull would when given the same query. Change this to use a ResultCollector derived from the query.

cc @buoyad

@mmaxim mmaxim requested a review from joshblum August 10, 2018 01:25
@keybase keybase deleted a comment from codecov bot Aug 10, 2018
@@ -165,7 +166,7 @@ func (s *baseConversationSource) postProcessThread(ctx context.Context, uid greg

func (s *baseConversationSource) TransformSupersedes(ctx context.Context,
unboxInfo types.UnboxConversationInfo, uid gregor1.UID, msgs []chat1.MessageUnboxed) ([]chat1.MessageUnboxed, error) {
transform := newBasicSupersedesTransform(s.G())
transform := newBasicSupersedesTransform(s.G(), false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on any of these calls when you pass unnamed value in to call this can you add /* useDeletePlaceholders */ so it's easier to read in the future?

@keybase keybase deleted a comment from codecov bot Aug 10, 2018
@mmaxim mmaxim merged commit f759f48 into master Aug 10, 2018
@mmaxim mmaxim deleted the mike/CORE-8523 branch August 10, 2018 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants