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

Use event streams to calculate presence #4942

Merged
merged 5 commits into from
Mar 28, 2019
Merged

Conversation

erikjohnston
Copy link
Member

@erikjohnston erikjohnston commented Mar 26, 2019

Primarily this fixes a bug in the handling of remote users joining a
room where the server sent out the presence for all local users in the
room to all servers in the room.

We also change to using the state delta stream, rather than the
distributor, as it will make it easier to split processing out of the
master process (as well as being more flexible).

Finally, when sending presence states to newly joined servers we filter
out old presence states to reduce the number sent. Initially we filter
out states that are offline and have a last active more than a week ago,
though this can be changed down the line.

Fixes #3962


Note that there a few TODO items, but I think they can be solved separately. Certainly I think what we have now is strictly better than what was there before.

@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #4942 into develop will increase coverage by 0.07%.
The diff coverage is 66.3%.

@@             Coverage Diff             @@
##           develop    #4942      +/-   ##
===========================================
+ Coverage    60.44%   60.52%   +0.07%     
===========================================
  Files          331      328       -3     
  Lines        34100    34173      +73     
  Branches      5627     5647      +20     
===========================================
+ Hits         20612    20683      +71     
+ Misses       12017    12011       -6     
- Partials      1471     1479       +8

@erikjohnston erikjohnston requested a review from a team March 26, 2019 16:36
@erikjohnston
Copy link
Member Author

I'm still figuring out quite how to test this fixes the bug, but at least we have sytests to check that the correct behaviour happens

Primarily this fixes a bug in the handling of remote users joining a
room where the server sent out the presence for all local users in the
room to all servers in the room.

We also change to using the state delta stream, rather than the
distributor, as it will make it easier to split processing out of the
master process (as well as being more flexible).

Finally, when sending presence states to newly joined servers we filter
out old presence states to reduce the number sent. Initially we filter
out states that are offline and have a last active more than a week ago,
though this can be changed down the line.

Fixes #3962
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

generally looks sane. a few nits.

synapse/handlers/presence.py Show resolved Hide resolved
synapse/handlers/presence.py Outdated Show resolved Hide resolved
tests/handlers/test_presence.py Outdated Show resolved Hide resolved
tests/handlers/test_presence.py Outdated Show resolved Hide resolved
tests/handlers/test_presence.py Outdated Show resolved Hide resolved
@@ -220,6 +220,15 @@ def __init__(self, hs):
LaterGauge("synapse_handlers_presence_wheel_timer_size", "", [],
lambda: len(self.wheel_timer))

# Used to handle sending of presence to newly joined users/servers
if hs.config.use_presence:
self.notifier.add_replication_callback(self.notify_new_event)
Copy link
Member

Choose a reason for hiding this comment

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

I'd love it if the stuff in the notifier could be renamed to be less about "replication", since we seem to be using it for things which aren't replication. Guess we can leave that for another time, though.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

synapse/handlers/presence.py Outdated Show resolved Hide resolved
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.

2 participants