Allow profile updates to happen on workers #3659
Conversation
| @@ -71,8 +71,6 @@ def get_from_remote_profile_cache(self, user_id): | |||
| desc="get_from_remote_profile_cache", | |||
| ) | |||
|
|
|||
|
|
|||
| class ProfileStore(ProfileWorkerStore): | |||
There was a problem hiding this comment.
Is there not a danger of races if the *_remote_profile_cache methods are called from a worker?
There was a problem hiding this comment.
on closer inspection, it looks like efforts are made to not call those methods from a worker, in which case I think they should probably stay in ProfileStore rather than moving.
synapse/handlers/profile.py
Outdated
| @@ -166,10 +171,16 @@ def set_displayname(self, target_user, requester, new_displayname, by_admin=Fals | |||
| ) | |||
|
|
|||
| if self.hs.config.user_directory_search_all_users: | |||
| profile = yield self.store.get_profileinfo(target_user.localpart) | |||
There was a problem hiding this comment.
I'm not entirely sure I'm happy that we end up with a UserDirectoryHandler which we then carefully don't use. Could we have some sort of stub handler which just provides a notify_profile_changed and is implemented by different classes on the workers than on the master?
synapse/handlers/profile.py
Outdated
| @@ -51,6 +52,10 @@ def __init__(self, hs): | |||
| self._start_update_remote_profile_cache, self.PROFILE_UPDATE_MS, | |||
There was a problem hiding this comment.
it might be a good time to shift this (and the things it calls) to a MasterProfileHandler
erikjohnston
force-pushed
the
erikj/split_profiles
branch
from
c3e6857
to
782689b
Compare
Turns out that the user directory handling is fairly racey as a bunch of stuff assumes that the processing happens on master, which it doesn't when there is a synapse.app.user_dir worker. So lets just call the function directly until we actually get round to fixing it, since it doesn't make the situation any worse.
synapse/handlers/profile.py
Outdated
| @@ -32,12 +32,12 @@ | |||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| class ProfileHandler(BaseHandler): | |||
| class WorkerProfileHandler(BaseHandler): | |||
| PROFILE_UPDATE_MS = 60 * 1000 | |||
synapse/handlers/profile.py
Outdated
| @@ -32,12 +32,12 @@ | |||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| class ProfileHandler(BaseHandler): | |||
| class WorkerProfileHandler(BaseHandler): | |||
There was a problem hiding this comment.
given that this is also used as a base class on the master instance, can you either give it a different name (BaseProfileHandler?) or a docstring that explains the situation?
| @@ -308,7 +308,10 @@ def build_initial_sync_handler(self): | |||
| return InitialSyncHandler(self) | |||
|
|
|||
| def build_profile_handler(self): | |||
| return ProfileHandler(self) | |||
| if self.config.worker_app: | |||
There was a problem hiding this comment.
could we have this return WorkerProfileHandler and then override it in synapse.server.HomeServer? possibly with a comment here to say that's what's going on?
There was a problem hiding this comment.
We could, but this is how we do it for other handlers. I'm not sure I really think it'll be clearer moving it into the actual app
There was a problem hiding this comment.
hrm, ok, let's punt it for now, though I'm not sure I agree.
There was a problem hiding this comment.
Certainly something we can revisit if/when we try and clean up the worker split out
synapse/handlers/profile.py
Outdated
| def __init__(self, hs): | ||
| super(MasterProfileHandler, self).__init__(hs) | ||
|
|
||
| self.clock.looping_call( |
There was a problem hiding this comment.
can we assert that hs.config.worker_app is None here?
|
woo, ta |
Based on #3632