Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Mark sync as limited if there is a gap in the timeline #16485

Merged
merged 7 commits into from
Oct 19, 2023

Conversation

erikjohnston
Copy link
Member

Fixes #16463

This splits thinsg into two queries, but most of the time we won't have
new event backwards extremities so this shouldn't actually add an extra
RTT for the majority of cases.

Note this removes the check for events with no prev events, but that was
part of MSC2716 work that has since been removed.
@erikjohnston erikjohnston marked this pull request as ready for review October 13, 2023 13:44
@erikjohnston erikjohnston requested a review from a team as a code owner October 13, 2023 13:44
Comment on lines +614 to +615
# If there is a gap then we need to only include events after
# it.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# If there is a gap then we need to only include events after
# it.
# If there is a gap then we need to include only events after
# it.

Maybe? I'm not sure why we would do this though?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we have a section of the timeline with events A, B, <gap>, X, Y, Z we need to make sure we only send down X, Y, Z and not A and B (which we currently do. If we did, then the client wouldn't try and paginate to fill in the gap.

synapse/handlers/sync.py Show resolved Hide resolved
synapse/handlers/sync.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/events.py Show resolved Hide resolved
synapse/storage/databases/main/stream.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/events.py Show resolved Hide resolved
@erikjohnston erikjohnston requested a review from clokep October 17, 2023 08:57
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

This should probably have a complement test?

@erikjohnston
Copy link
Member Author

This should probably have a complement test?

Can someone write a bot to remind me about tests being a thing??

@erikjohnston
Copy link
Member Author

@clokep I wrote a test and it found an edge case! I've hopefully fixed it....

@erikjohnston erikjohnston requested a review from clokep October 18, 2023 14:33
@erikjohnston erikjohnston merged commit e9069c9 into develop Oct 19, 2023
38 checks passed
@erikjohnston erikjohnston deleted the erikj/sync_backfill_limited branch October 19, 2023 14:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/sync doesn't inform clients when there is a gap in the timeline
2 participants