Only accept invites on one worker, which can be optionally designated #9
Conversation
| ) or (config.worker_to_run_on == self._api.worker_name) | ||
|
|
||
| if not should_run_on_this_worker: | ||
| logger.info( |
There was a problem hiding this comment.
This (and the logging below it) feel a bit debuggey to me.
There was a problem hiding this comment.
I was kind of mixed on that. It doesn't feel verbose enough to be debug; it's a one-off message on startup and it tends to be the kind of thing that will really help someone out if it's not working.
On the other hand, debug output is so verbose that I don't think many people run with it enabled.
However if you really think so anyway then I can change it
| # (For workerised Synapse deployments) | ||
| # If you want to accept invites on a specific worker, specify its instance | ||
| # name here. Otherwise, invites will be processed on the main process. |
There was a problem hiding this comment.
Is there an ideal process to run this on? (I.e. to avoid replication?) Can we figure this out without asking people to configure more bits? Workers are already quite confusing!
There was a problem hiding this comment.
The only benefit really would be one of the account data stream writers since it will save a replication trip for DMs (in conjunction with #6), but we're being so pedantic at that point that I thought it would make it more complicated to document that. However we can't autoconfigure that since we don't know the name of an active account data worker.
There's always replication (event creators, event persisters) in the mix somewhere.
Relying on the 'main' process has the advantage of simplicity in that we know there will be exactly one of them and we know it doesn't have a name; it's also fairly understandable that things will break if it's offline.
As you say, workers are confusing, so making it not a problem to configure in the general case seems ideal, hence the default here.
Co-authored-by: Patrick Cloke <[email protected]>
reivilibre
force-pushed
the
rei/on_one_worker
branch
from
5f5e96e to
ddaaef2
Compare
Co-authored-by: Brendan Abolivier <[email protected]>
| ### A note about logging | ||
|
|
||
| Your Synapse logging configuration should have the following option set in it: | ||
|
|
||
| ```yaml | ||
| disable_existing_loggers: False | ||
| ``` | ||
|
|
||
| Without it, logging from this module (and potentially others) may not appear in your logs. | ||
|
|
There was a problem hiding this comment.
What's special about modules that makes this necessary?
There was a problem hiding this comment.
Not sure, I find myself having to use this incantation is basically every Python project ever. but somehow Synapse avoids it for itself and Twisted.. :/
There was a problem hiding this comment.
I don't think synapse does avoid it... that's why it's included in https://github.com/matrix-org/synapse/blob/master/docs/sample_log_config.yaml#L84
There was a problem hiding this comment.
(the problem applies generally to python modules that are loaded before the logger is configured.)
There was a problem hiding this comment.
Ah, I see, so some of Synapse's loggers would be getting disabled. Good to know, thanks!
Not sure why my config didn't have it.
I have to say I kind of find this flag stupid, but oh well I'm sure someone had some reason once upon a time. It may as well say dont_work_properly: False.
There was a problem hiding this comment.
Not sure why my config didn't have it.
back in the very early days we didn't recommend it (see matrix-org/synapse#6197).
It may as well say
dont_work_properly: False.
agreed. "something something backwards compatibility", I expect. TBF, it's not entirely clear that the authors of logging.config.dictConfig ever expected applications to pipe user configuration straight into it. Maybe we should set disable_existing_loggers: True in the dict before we pass it through.
|
Removed my review -- this is already approved. |
The main process was chosen since it's a good default as we know that it exists and what name it has.
For full flexibility, we also give a config option to let the admin choose a worker.
Fixes #8.
It should be possible to review commit-by-commit.