This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Deduplicate presence updates before sending them to application services #11140
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anoadragon453
added a commit
that referenced
this pull request
Oct 21, 2021
We calculate presence updates for application services based on the users that application service is interested in. For each of these users, we determine which presence updates they are set to receive, compile that into a list, and then send every update from the list to the application service. However, because a single presence update can cause a notification for many different users, we're likely to end up with lots of duplicated presence updates being collected here. Currently, all of these are sent to the application service. By using a Set, this deduplication happens automatically.
anoadragon453
force-pushed
the
anoa/presence_events_as_set
branch
from
f388592
to
8a249da
Compare
This wasn't necessary, but mypy complained that 'events' could be both a List and a Set of JsonDict. So rather than define it with a Union type as such, I figured it'd be best to convert the other methods to use Set as well. We may even eliminate other duplicates in the process.
clokep
reviewed
Jan 14, 2022
| @@ -427,7 +427,7 @@ async def _handle_presence( | |||
| from_key=from_key, | |||
| ) | |||
| time_now = self.clock.time_msec() | |||
| events.extend( | |||
| events.update( | |||
There was a problem hiding this comment.
Should this really be a set or do we want to de-duplicate based on user ID?
| @@ -491,7 +491,7 @@ async def get_new_events_as( | |||
| ): | |||
| continue | |||
|
|
|||
| events.append(self._make_event_for(room_id)) | |||
| events.add(self._make_event_for(room_id)) | |||
There was a problem hiding this comment.
This feels like it being a set might not de-duplicate a lot since the object embeds a list of user IDs?
|
Looks like we forgot about this one. Is it worth resurrecting @anoadragon453? |
|
Let's close it for now since it's ancient. One day I may return to finish it up. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mostly addresses problem 2 of #10836 (comment).
We calculate presence updates for application services based on the
users that application service is interested in. For each of these
users, we determine which presence updates they are set to receive,
compile that into a list, and then send every update from the list
to the application service.
However, because a single presence update can cause a notification
for many different users, we're likely to end up with lots of
duplicated presence updates being collected here. Currently, all of
these are sent to the application service.
By using a Set, this deduplication happens automatically.