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

Add room_types/not_room_types filtering to Sliding Sync /sync #17337

Merged

Conversation

MadLittleMods
Copy link
Collaborator

@MadLittleMods MadLittleMods commented Jun 19, 2024

Add room_types/not_room_types filtering to Sliding Sync /sync

Based on MSC3575: Sliding Sync

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

Comment on lines 439 to 440
# FIXME: Do we have to worry about gaps? What happens if we try to get a point
# in the room we haven't backfilled before?
Copy link
Collaborator Author

@MadLittleMods MadLittleMods Jun 19, 2024

Choose a reason for hiding this comment

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

This doesn't affect this PR because we're just looking at the create event which we will always have but I'm curious about this for get_state_at(...) in general.

For state in a room, do we have to worry about gaps? What happens if we try to get state a point in the room we haven't backfilled before?

I guess I just don't know if we have all historical state in the room. I know we have the whole auth chain of things which will be a subset of state 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@erikjohnston Is this a legit concern?

Copy link
Collaborator Author

@MadLittleMods MadLittleMods Jun 25, 2024

Choose a reason for hiding this comment

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

Talked with @erikjohnston and got some knowledge.

In general, asking for state at a random position is dubious and we should careful about our usages.

It's not a problem in our current Sliding Sync usages because we're using it with the to_token which is at the front of the room so there will always be some latest event before the to_token to get state at that positon.

@MadLittleMods MadLittleMods marked this pull request as ready for review June 20, 2024 03:11
@MadLittleMods MadLittleMods requested a review from a team as a code owner June 20, 2024 03:11
@MadLittleMods MadLittleMods merged commit 7be03d8 into develop Jul 2, 2024
39 checks passed
@MadLittleMods MadLittleMods deleted the madlittlemods/msc3575-sliding-sync-filter-room-types branch July 2, 2024 17:46
@MadLittleMods
Copy link
Collaborator Author

Thanks for the review @erikjohnston 🦅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants