-
Notifications
You must be signed in to change notification settings - Fork 448
Support sending and receiving MSC4354 Sticky Event metadata. #19365
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
base: develop
Are you sure you want to change the base?
Conversation
2b83332 to
53d4486
Compare
e49d4bd to
78ee6e8
Compare
Including delayed events
78ee6e8 to
fb67e9a
Compare
|
|
||
| class StickyEvent: | ||
| QUERY_PARAM_NAME: Final = "org.matrix.msc4354.sticky_duration_ms" | ||
| FIELD_NAME: Final = "msc4354_sticky" |
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.
| FIELD_NAME: Final = "msc4354_sticky" | |
| EVENT_FIELD_NAME: Final = "msc4354_sticky" |
| Maximum stickiness duration as specified in MSC4354. | ||
| Ensures that data in the /sync response can go down and not grow unbounded. | ||
| """ | ||
| MAX_EVENTS_IN_SYNC: Final = 100 |
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.
Why 100? (we should explain in the attribute docstring)
(also not used yet)
|
|
||
|
|
||
| class StickyEvent: | ||
| QUERY_PARAM_NAME: Final = "org.matrix.msc4354.sticky_duration_ms" |
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.
We should explain what endpoint this applies to
|
|
||
| class StickyEvent: | ||
| QUERY_PARAM_NAME: Final = "org.matrix.msc4354.sticky_duration_ms" | ||
| FIELD_NAME: Final = "msc4354_sticky" |
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.
The separation between StickyEventField and StickyEvent.FIELD_NAME is slightly mind bending.
| if self._msc4354_enabled and event.sticky_duration(): | ||
| # The de-outliered event is sticky. Update the sticky events table to ensure | ||
| # we deliver this down /sync. | ||
| self.store.insert_sticky_events_txn(txn, [event]) |
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.
Are we going to give up on events that were valid sticky events sent within the hour but before msc4354_enabled?
Seems like a fine trade-off but we should at-least comment about it.
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.
Hmm. Didn't consider that, not too worried about it, but on the other hand we could track sticky events even if we don't deliver them, whilst the feature is turned off.
Thoughts overall?
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.
No specific thoughts. Both have tradeoffs. With the current setup, you can completely turn the feature off and I think it's pretty acceptable to drop them on the floor since this an hour window max. But it could also just make more sense for things to behave the same.
For things like the Sliding Sync feature, we had to think about this a lot harder as there was no time limit.
| event_dict[StickyEvent.FIELD_NAME] = { | ||
| "duration_ms": event.sticky_duration_ms, | ||
| } |
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.
Seems like we should re-use StickyEventField here
(applies to other places)
| f"{EventUnsignedContentFields.STICKY_TTL} field unexpectedly found in {event_id}: {event}", | ||
| ) | ||
|
|
||
| def test_sticky_event_via_event_endpoint(self) -> None: |
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.
We should also have a test for when a sticky event is sent but msc4354_enabled: False
| @attr.s(slots=True, frozen=True, auto_attribs=True) | ||
| class EventDetails: | ||
| room_id: RoomID | ||
| type: EventType | ||
| state_key: StateKey | None | ||
| origin_server_ts: Timestamp | None | ||
| content: JsonDict | ||
| device_id: DeviceID | None | ||
| sticky_duration_ms: int | None | ||
|
|
||
|
|
||
| @attr.s(slots=True, frozen=True, auto_attribs=True) | ||
| class DelayedEventDetails(EventDetails): | ||
| delay_id: DelayID | ||
| user_localpart: UserLocalpart |
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.
I haven't looked into the details but should we have a similarly named StickyEventDetails that includes sticky_duration_ms instead of in EventDetails
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.
I think this is purely tracking the sticky duration so that when we send the delayed event, we can mark it as sticky at that time.
Interestingly DelayedEventDetails inherits from EventDetails but there are no standalone usage of EventDetails.
I guess the idea was just to compartmentalise 'this is information for sending the event' and 'this is the information for scheduling it', then again I don't know why the user localpart ( = sender ?) is stored on the delayed event side then.
Anyway, to me it seems reasonable to have it be part of the event details themselves. If we had StickyEventDetails in addition, DelayedEventDetails would have to inherit from that and I don't think the extra layer would help us with anything. (maybe I'm missing something from this design though)
Co-authored-by: Eric Eastwood <erice@element.io>
1c2a9ad to
beead8a
Compare
Part of: MSC4354
Follows: #19340 (a necessary bugfix for
/event/to set this metadata)Partially supersedes: #18968
This PR implements the first batch of work to support MSC4354 Sticky Events.
Sticky events are events that have been configured with a finite 'stickiness' duration,
capped to 1 hour per current MSC draft.
Whilst an event is sticky, we provide stronger delivery guarantees for the event, both to
our clients and to remote homeservers, essentially making it reliable delivery as long as we
have a functional connection to the client/server and until the stickiness expires.
This PR merely supports creating sticky events and receiving the sticky TTL metadata in clients.
It is not suitable for trialling sticky events since none of the other semantics are implemented.
The current plan is to follow this PR up with more PRs, roughly parcelled up as follows:
Add MSC4354 experimental feature flag
Expose MSC4354 enablement on /versions
Add constants for sticky events
Add sticky_events table
Add sticky events store and stream
EventBase: add the concept of sticky_duration
EventBuilder: allow building events with sticky event fields
store method: insert_sticky_events_txn
When persisting currently-sticky events, add to sticky event stream
Allow clients to send sticky events
Including delayed events
Add test helper for sending sticky events
Expose the sticky event TTL to clients
Add a test for sticky TTL calculation and exposure to clients