-
Notifications
You must be signed in to change notification settings - Fork 243
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
Sliding sync: use new DB tables #17630
Conversation
Co-authored-by: Eric Eastwood <[email protected]>
Co-authored-by: Eric Eastwood <[email protected]>
3f546ec
to
e76954b
Compare
a26f46d
to
e923a8d
Compare
) -> Dict[str, RoomsForUserSlidingSync]: | ||
sql = """ | ||
SELECT m.room_id, m.sender, m.membership, m.membership_event_id, | ||
r.room_version, |
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.
It looks like we're simply fetching out room_version_id
just because that's what's in a RoomsForUser
. We don't use it anywhere in the Sliding Sync code (as far as I can tell).
- We can remove it from
RoomsForUserSlidingSync
- We should just use a different data type. Probably the same as
RoomsForUser
but withoutroom_version_id
. We could haveRoomsForUserSlidingSync
andRoomsForUserSlidingSyncWithState
for example
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.
This could be addressed in a future PR. Probably would be good to create an issue for this one
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.
Ah, yeah. So, I had the same thought about removing it, but it turns out that sync v2 specifically checks that the room version is known and so we should probably do something similar in sliding sync as well.
Broke in #17630 --------- Co-authored-by: Andrew Morgan <[email protected]>
…ng_sync_rooms_for_user(...)` (#17692) We need to bust the `get_sliding_sync_rooms_for_user` cache when the room encryption is updated and any other field that is used in the query. Follow-up to #17630 - Bust cache for membership change (cross-reference `get_rooms_for_user`) - Bust cache for room `encryption` (cross-reference `get_room_encryption`) - Bust cache for `forgotten` (cross-reference `did_forget`/`get_forgotten_rooms_for_user`)
Move filters tests to rest layer in order to test the new (with sliding sync tables) and fallback paths that Sliding Sync can use. Also found a bug in the new path because it's not being tested which is also fixed in this PR. We now take into account `has_known_state` when filtering. Spawning from #17662 (comment). This should have been done when we started using the new sliding sync tables in #17630
Based on #17629
Utilizing the new sliding sync tables added in #17512 for fast acquisition of rooms for the user and filtering/sorting.