Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Replay time ordering #405
Replay time ordering #405
Changes from 13 commits
83ace70
233ccc2
3b84c99
2e7bbbd
109283d
9ee710f
dcd18b8
531feb1
ce6fb3a
19dfb83
b6a5ee8
cc8b817
161cba4
8d7bdd6
a214c59
2d22f43
9b1432c
a3818cf
63d548d
04bae9e
910d489
ad35612
1cd42d9
b138b15
1f5c6d0
1c7ca58
d3c24f7
f06ebe5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok so I guess the approach now is:
• change
foreground = true
to also useupdatedAtMsDiff
(for consistency? I might have been the one to bring this up?)• Don't special case
verified = true
on the client side, handle it differently on the server sideIf we decided not to change
foreground = true
toupdatedAtMsDiff
, would the other changes toupdatedAtMsDiff
in these PRs be enough to fix the bug? Or is the inconsistent behavior betweenforeground = true
andforeground = false
causing a problem?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for three reasons:
updatedAtMsDiff
causes a problem when a foreground location gets forced to server time while background locations are adjusted perupdatedAtMsDiff
, leading to inconsistent ordering (a subsequent background location can be brought erroneously before the foreground location by theupdatedAtMsDiff
adjustment). So no, the otherupdatedAtMsDiff
changes would be not sufficient to fully fix the bug.now
timestampThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, per slack conversation. I'll link in #mobile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it would be nice if iOS and Android had the same logic for this
if
caseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout, fixed: 1cd42d9