Expose MSC4354 Sticky Events over MSC4186 (Simplified) Sliding Sync.#19591
Conversation
reivilibre
changed the title
reivilibre
force-pushed
the
rei/sticky_events_sliding_sync
branch
4 times, most recently
from
360c60b to
adb3191
Compare
reivilibre
force-pushed
the
rei/sticky_events_sliding_sync
branch
from
97efd43 to
6883dcd
Compare
Fixes builtins.NotImplementedError: Cannot check isinstance when validating from json, use a JsonOrPython validator instead.
This reverts commit 6883dcd. EPEL 10 only had 2.9.2 so try to work around issue without updating
builtins.NotImplementedError: Cannot check isinstance when validating from json, use a JsonOrPython validator instead.
reivilibre
force-pushed
the
rei/sticky_events_sliding_sync
branch
from
6883dcd to
c5c1b01
Compare
reivilibre
requested review from
a team and
erikjohnston
and removed request for
erikjohnston
| since_token = sticky_events_request.since or SlidingSyncStickyEventsToken( | ||
| sticky_events_stream_id=0 | ||
| ) | ||
| ( | ||
| sticky_events_to_id, | ||
| room_to_event_ids, | ||
| ) = await self.store.get_sticky_events_in_rooms( | ||
| all_interested_room_ids, | ||
| from_id=since_token.sticky_events_stream_id, | ||
| to_id=to_token.sticky_events_key, | ||
| now=now, | ||
| limit=min(sticky_events_request.limit, StickyEvent.MAX_EVENTS_IN_SYNC), | ||
| ) |
There was a problem hiding this comment.
The Sticky Events extension having it's own pagination inside Sliding Sync seems like a smell to me.
Related discussion: matrix-org/matrix-spec-proposals#4354 (comment)
There was a problem hiding this comment.
I can't find the linked discussion (GH deep links not working atm) but I trust that it captures the essence of the smell enough?
I broadly agree; in fact I'd go so far as to say that sliding sync is missing a high-level steward who sets the strategy for pagination. I have opened matrix-org/matrix-spec#2378 to describe this but not sure how to 'anchor' this down anywhere or find the right outcome for it.
There was a problem hiding this comment.
Thanks for creating the issue. Given the to_device extension already uses this pattern, I guess we can opt to use it as well. Holding on to the hope that we don't settle.
I'm not really convinced it's the way to go. It's basically introducing it's own pagination behavior compared to the normal part of the Sliding Sync response. For the normal Sliding Sync response, the "No pagination" pattern is just to return all of the latest unseen data and if there is too much data (they've been offline too long and the homeserver deems it will take too many resources to compile their response, or the response is too large), reset the connection (M_UNKNOWN_POS) which tells the client to start over. I think that last detail should be added to matrix-org/matrix-spec#2378
And my general thoughts for sticky events is summarized in matrix-org/matrix-spec-proposals#4354 (comment) which was resolved for some reason 😠
reivilibre
force-pushed
the
rei/sticky_events_sliding_sync
branch
from
f5b8de3 to
aa20913
Compare
| since_token = sticky_events_request.since or SlidingSyncStickyEventsToken( | ||
| sticky_events_stream_id=0 | ||
| ) | ||
| ( | ||
| sticky_events_to_id, | ||
| room_to_event_ids, | ||
| ) = await self.store.get_sticky_events_in_rooms( | ||
| all_interested_room_ids, | ||
| from_id=since_token.sticky_events_stream_id, | ||
| to_id=to_token.sticky_events_key, | ||
| now=now, | ||
| limit=min(sticky_events_request.limit, StickyEvent.MAX_EVENTS_IN_SYNC), | ||
| ) |
There was a problem hiding this comment.
Thanks for creating the issue. Given the to_device extension already uses this pattern, I guess we can opt to use it as well. Holding on to the hope that we don't settle.
I'm not really convinced it's the way to go. It's basically introducing it's own pagination behavior compared to the normal part of the Sliding Sync response. For the normal Sliding Sync response, the "No pagination" pattern is just to return all of the latest unseen data and if there is too much data (they've been offline too long and the homeserver deems it will take too many resources to compile their response, or the response is too large), reset the connection (M_UNKNOWN_POS) which tells the client to start over. I think that last detail should be added to matrix-org/matrix-spec#2378
And my general thoughts for sticky events is summarized in matrix-org/matrix-spec-proposals#4354 (comment) which was resolved for some reason 😠
Co-authored-by: Eric Eastwood <erice@element.io>
3 participants
Follows: #19487
Part of: MSC4354 whose experimental feature tracking issue is #19409
This PR implements the Sliding Sync (MSC4186) extension described in MSC4354, allowing sliding sync clients
to receive sticky events in a reliable way.
The logic is much the same as for oldschool sync (implementation in #19487),
although in the sliding sync extension, the client can choose their own limit
and must control their own pagination through an extra token in the extension request/response bodies.
EDIT: Note this PR does not yet send down existing sticky events in the room when the room has been newly-joined.
This newly-discovered gap is tracked at #19662 and will be addressed for both current sync and MSC4186 SSS soon.
This pull request is commit-by-commit review friendly.
Add gather_optional_coroutines/7 overload
Add fields for sticky events sliding sync extension
Add explicit Absent utility type
Implement sliding sync extension for sticky events
Add sliding sync extension test
drive-by docstring tweak on ordering