-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Allow multiple workers to write to receipts stream. #16432
Conversation
5bf812b
to
0ae622f
Compare
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.
(Sorry, I couldn't help myself from looking...)
@overload | ||
def on_new_event( | ||
self, | ||
stream_key: Literal[StreamKeyType.ROOM], | ||
new_token: RoomStreamToken, | ||
users: Optional[Collection[Union[str, UserID]]] = None, | ||
rooms: Optional[StrCollection] = None, | ||
) -> 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.
I'm surprised we didn't already need this overload! Does it help avoid any asserts or anything?
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 doesn't reduce assertions, as it only limits what is accepted as arguments (it doesn't change the return type). I added it to catch the case where we passed in an int
when using StreamKeyType.RECEIPT
.
if len(self.writers.receipts) == 0: | ||
raise ConfigError( | ||
"Must only specify one instance to handle `receipts` messages." | ||
"Must specify at least one instance to handle `receipts` messages." |
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 update the documentation for this?
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'm somewhat inclined to leave this undocumented until we can test it? But I could also be persuaded otherwise.
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.
That's fine. 👍
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 reasonable? Do we have tests for this? Do we need to update sytest / complement?
Whoops! Forgot about that. Have added tests, though it fails due to #16473 |
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.
Do we also need to update sytest or complement to have multiple workers for this?
@@ -0,0 +1,243 @@ | |||
# Copyright 2020 The Matrix.org Foundation C.I.C. |
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.
# Copyright 2020 The Matrix.org Foundation C.I.C. | |
# Copyright 2023 The Matrix.org Foundation C.I.C. |
Fixes #16417
Reviewable commit-by-commit.