Make it more clear how shared_extra_conf is combined in our Docker configuration scripts (#19323)
For reference, this PR used to include this whole `shared_config` block in the diff. But https://github.com/element-hq/synapse/pull/19324 was merged first which introduced parts of it already. Here is what this code used to look like: https://github.com/element-hq/synapse/blob/566670c363915691826b5b435c4aa7acde61b408/docker/configure_workers_and_start.py#L865-L868 --- Original context for why it was changed this way: https://github.com/matrix-org/synapse/pull/14921#discussion_r1126257933 Previously, this code made me question two things: 1. Do we actually use `worker_config["shared_extra_conf"]` in the templates? - At first glance, I couldn't see why we're updating `shared_extra_conf` here. It's not used in the `worker.yaml.j2` template so all of this seemed a bit pointless. - Turns out, updating `shared_extra_conf` itself is pointless and it's being done as a convenient place to mix the objects to get things right in `shared_config` (confusing). 1. Does it actually do anything? - Because `shared_config` starts out as an empty object, my first glance made me think we we're just updating with an empty object and then just re-assigning. But because we're in a loop, we actually accumulate the `shared_extra_conf` from each worker. I'm not sure whether I'm capturing my confusion well enough here but basically, this made me spend time trying to figure out what/why we're doing things this way and we can use a more clear pattern to accomplish the same thing. --- This change is spawning from looking at the `docker/configure_workers_and_start.py` script in order to add a metrics listener ([upcoming PR](https://github.com/element-hq/synapse/pull/19324)).
This commit is contained in:
@@ -0,0 +1 @@
|
||||
Make it more clear how `shared_extra_conf` is combined in our Docker configuration scripts.
|
||||
@@ -879,8 +879,8 @@ def generate_worker_files(
|
||||
# worker.
|
||||
shared_config = {
|
||||
**worker_config["shared_extra_conf"],
|
||||
# We combine `shared_config` second to avoid overwriting existing keys
|
||||
# because TODO: why?
|
||||
# We combine `shared_config` second to avoid overwriting existing keys just
|
||||
# for sanity sake (always use the first worker).
|
||||
**shared_config,
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user