Create a worker for event creation #2854
Conversation
erikjohnston
changed the title
|
Please add documentation for this worker to https://github.com/matrix-org/synapse/blob/develop/docs/workers.rst (including the routes that need to be redirected to it). |
erikjohnston
force-pushed
the
erikj/event_create_worker
branch
9 times, most recently
from
3e8e1a3 to
ba52796
Compare
The intention was for the check to be called as early as possible in the request, but actually was called just before the main ratelimit check, so was fairly pointless.
erikjohnston
force-pushed
the
erikj/event_create_worker
branch
from
ba52796 to
b7d9248
Compare
As we want to have it run on the main synapse instance
erikjohnston
force-pushed
the
erikj/event_create_worker
branch
from
b7d9248 to
f133228
Compare
There was a problem hiding this comment.
this generally looks sane, but it feels to me like the "http replication" we're introducing here is quite orthogonal to the existing TCP replication (and the old HTTP replication we used to have), and isn't really replication at all.
could we call it the "worker interface" or something?
| self.worker_replication_port = config.get("worker_replication_port", None) | ||
|
|
||
| # The port on the main synapse for HTTP replication endpoint | ||
| self.worker_replication_http_port = config.get("worker_replication_http_port") |
There was a problem hiding this comment.
does this need documenting in workers.rst ?
| @@ -219,6 +220,9 @@ def _configure_named_resource(self, name, compress=False): | |||
| if name == "metrics" and self.get_config().enable_metrics: | |||
| resources[METRICS_PREFIX] = MetricsResource(self) | |||
|
|
|||
| if name == "replication": | |||
|
|
||
| Handles non-state event creation. It can handle REST endpoints matching: | ||
|
|
||
| ^/_matrix/client/(api/v1|r0|unstable)/rooms/.*/send |
There was a problem hiding this comment.
Shouldn't this also be handling the macro endpoints for creating events (https://matrix.org/docs/spec/client_server/r0.3.0.html#post-matrix-client-r0-rooms-roomid-ban etc)? Or are they deliberately being left on the master as they're not that common?
There was a problem hiding this comment.
We're currently only handling non-state events on this worker for now, as sometimes the membership state changes end up doing complicated things that probably need to be done on the master.
I've taken replication to mean all internal traffic between two nodes, both downstream and upstream communications. I can changes the name I guess, but it means internal communication is now under two names rather than one. |
There was a problem hiding this comment.
ok it seems to make sense now you've documented that the interfaces are complementary rather than alternatives.
lgtm modulo a couple of doc niggles as below.
I wouldn't mind seeing a bit of rationalisation in https://github.com/matrix-org/synapse/blob/master/docs/replication.rst and https://github.com/matrix-org/synapse/blob/master/docs/tcp_replication.rst, but happy for this to be punted.
docs/workers.rst
Outdated
| unencrypted. | ||
|
|
||
| (Roughly, the TCP port is used for streaming data from the master to the | ||
| workers, and the HTTP port for the workers to communicate with the main |
There was a problem hiding this comment.
s/communicate with/send data to/
docs/workers.rst
Outdated
| replication endpoints that it's talking to on the main synapse process. | ||
| ``worker_replication_host`` should specify the host of the main synapse, | ||
| ``worker_replication_port`` should point to the TCP replication listener port and | ||
| ``worker_replication_http_port`` should point to the HTTP replication port. | ||
|
|
There was a problem hiding this comment.
might be worth mentioning here that only the event_creator worker needs this for now, to help people upgrading.
Includes
#2847#2856