Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Allow multiple workers to write to receipts stream. #16432

Merged
merged 15 commits into from Oct 25, 2023

Conversation

erikjohnston
Copy link
Member

Fixes #16417

Reviewable commit-by-commit.

Copy link
Contributor

@clokep clokep left a 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...)

synapse/types/__init__.py Show resolved Hide resolved
synapse/types/__init__.py Show resolved Hide resolved
@erikjohnston erikjohnston marked this pull request as ready for review October 6, 2023 10:17
@erikjohnston erikjohnston requested a review from a team as a code owner October 6, 2023 10:17
Comment on lines +458 to +466
@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:
...
Copy link
Contributor

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?

Copy link
Member Author

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.

Comment on lines +361 to +363
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."
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. 👍

Copy link
Contributor

@clokep clokep left a 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?

@erikjohnston
Copy link
Member Author

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

Copy link
Contributor

@clokep clokep left a 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2020 The Matrix.org Foundation C.I.C.
# Copyright 2023 The Matrix.org Foundation C.I.C.

@erikjohnston erikjohnston merged commit ba47fea into develop Oct 25, 2023
41 checks passed
@erikjohnston erikjohnston deleted the erikj/multi_receipts2 branch October 25, 2023 15:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Receipts worker needs to be sharded
2 participants