Skip to content

Only advance the limbo free snapshot if query is synced #699

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

Merged

Conversation

schmidt-sebastian
Copy link
Contributor

Despite what we talked about offline, I still think we need to only advance the last limbo free snapshot version if the query is CURRENT. This is because we only calculate Limbo documents when we are in sync in the server. While we don't persist resume tokens for queries that aren't CURRENT, we do persist the remaining query data (including the lastLimboFreeSnapshot). We should not do that unless we are CURRENT.

Despite what we talked about offline, I still think we need to only advance the last limbo free snapshot version if the query is CURRENT. This is because we only calculate Limbo documents when we are in sync in the server. While we don't persist resume tokens for queries that aren't CURRENT, we do persist the remaining query data (including the lastLimboFreeSnapshot). We should not do that unless we are CURRENT.
Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

Ah, I see: in updateLimboDocuments() the first thing we do is short circuit and return the empty list if not current.

LGTM

@wilhuff wilhuff assigned schmidt-sebastian and unassigned wilhuff Aug 14, 2019
@schmidt-sebastian schmidt-sebastian merged commit aafbec4 into mrschmidt/indexfree-master Aug 14, 2019
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/indexfree-synced branch August 27, 2019 23:45
@firebase firebase locked and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Override cla size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants