Refactor the `configure_workers_and_start.py` script used internally by Complement. by reivilibre · Pull Request #16803 · element-hq/synapse

reivilibre · 2024-01-10T12:15:19Z

Revival of matrix-org/synapse#16650

Base: develop

Original commit schedule, with full messages:

Remove obsolete "app" from worker templates
Convert worker templates into dataclass
Use a lambda for the worker name rather than search and replace later
Collapse WORKERS_CONFIG by removing entries with defaults
Convert listener_resources and endpoint_patterns to Set[str]
Tweak comments
Add merge_into
Remove special logic for adding stream_writers: just make it part of the extra config template
Rename function to add_worker_to_instance_map given reduction of scope
Add sharding_allowed to the WorkerTemplate rather than having a separate function for that
Use merge_into when adding workers to the shared config
Promote mark_filepath to constant
Add a --generate-only option
Docstring on WorkerTemplate
Fix comment and mutation bug on merge_worker_template_configs
Update comment on merged
Tweak instantiate_worker_template, both in name, description and variable names

…the extra config template

…arate function for that

…riable names

Signed-off-by: Olivier Wilkinson (reivilibre) <oliverw@matrix.org>

erikjohnston · 2024-01-16T10:59:12Z

docker/configure_workers_and_start.py

        "worker_extra_conf": "",
    },
    "media_repository": {
-        "app": "synapse.app.generic_worker",


Hmm, the documentation still thinks that you should use synapse.app.media_repository to configure this: https://element-hq.github.io/synapse/latest/workers.html#synapseappmedia_repository

Hrm, that's a headscratcher.

Complement does test the media endpoints and the tests pass.......?

But looking at

synapse/synapse/config/repository.py

Line 129 in f95cfd8

and config.get("worker_app") != "synapse.app.media_repository"

(the only place where synapse.app.media_repository seems to matter), you'd expect this to break things...

erikjohnston

Seems mostly sane, though this is a large PR.

Is this just refactoring to make things easier or was this blocking something?

docker/configure_workers_and_start.py

erikjohnston · 2024-01-16T11:12:04Z

docker/configure_workers_and_start.py

+    """
+    Merges `new` into `dest` with the following rules:
+
+    - dicts: values with the same key will be merged recursively


In general I'm very cautious about this, as deep merging can be quite a foot gun. Sometimes it makes sense to merge configuration dicts, sometimes it really doesn't and can be very surprising.

do you have any examples of where it doesn't make sense? I can't remember ever seeing one (and I use NixOS which is all about merging config dicts...!), but on the other hand I have been bitten by dicts not being merged quite a few times :).

The common case would be e.g. database config, where you'd have some config for a sqlite db and then it gets merged into config for a postgres db, and now you have a config with mixed parameters from both. I believe nixos gets around this by erroring out if the same key is set to different values in both dicts

I mean yes I guess, but I don't consider that merging if two different values have been specified for the same key (it will raise an exception for this case, see https://github.com/element-hq/synapse/pull/16803/files#diff-30887e6a3a63f861b737500ca8243d3da5f0e7df016cc9cc020b8eeb61c2320cR355-R356).

erikjohnston · 2024-01-16T11:14:02Z

docker/configure_workers_and_start.py

    "event_persister": WorkerTemplate(
        listener_resources={"replication"},
+        shared_extra_conf=lambda worker_name: {
+            "stream_writers": {"events": [worker_name]}


Rather than trying to recursively merge stream writers, can we instead add a top level config to WorkerTemplate?

yeah, if that works smoothly I will give it a go.

erikjohnston · 2024-01-16T11:16:41Z

docker/configure_workers_and_start.py

    sys.stderr.flush()


+def merge_into(dest: Any, new: Any) -> None:


The name into makes me expect the second param is what is being changed, i.e dest into new. Maybe merge_update?

tempted just to replace its use with merged and have merged leave the input untouched

docker/configure_workers_and_start.py

…id footguns

Pulled out of #16803 since the drive-by cleanup was maybe not as drive-by as I had hoped.     Base: `develop`    Original commit schedule, with full messages: <ol> <li> Add a --generate-only option </li> </ol> --------- Signed-off-by: Olivier Wilkinson (reivilibre) <oliverw@matrix.org>

reivilibre added 18 commits November 17, 2023 12:03

Remove obsolete "app" from worker templates

b39a50a

Convert worker templates into dataclass

a22eb7d

Use a lambda for the worker name rather than search and replace later

ba3b6a4

Collapse WORKERS_CONFIG by removing entries with defaults

67d4fc8

Convert listener_resources and endpoint_patterns to Set[str]

94a85b3

Tweak comments

26073fa

Add merge_into

8b74639

Remove special logic for adding stream_writers: just make it part of …

f38297b

…the extra config template

Rename function to add_worker_to_instance_map given reduction of scope

7d8824e

Add sharding_allowed to the WorkerTemplate rather than having a sep…

f49dbc7

…arate function for that

Use merge_into when adding workers to the shared config

3bb21a9

Promote mark_filepath to constant

fbafde8

Add a --generate-only option

321d359

Docstring on WorkerTemplate

259a808

Fix comment and mutation bug on merge_worker_template_configs

3a46cf0

Update comment on merged

2f1d727

Tweak instantiate_worker_template, both in name, description and va…

ad4bb0e

…riable names

Newsfile

2ff1de3

Signed-off-by: Olivier Wilkinson (reivilibre) <oliverw@matrix.org>

reivilibre marked this pull request as ready for review January 10, 2024 14:36

reivilibre requested a review from a team as a code owner January 10, 2024 14:36

erikjohnston reviewed Jan 16, 2024

View reviewed changes

Move stream_writers to their own field in the WorkerTemplate

29541fd

reivilibre force-pushed the rei/cwas_extension branch from 8c71575 to 8c72abc Compare January 17, 2024 17:17

Remove merge_into and just have merged which copies inputs to avo…

c91ab4b

…id footguns

reivilibre force-pushed the rei/cwas_extension branch from 8c72abc to c91ab4b Compare January 17, 2024 17:18

This was referenced Jan 18, 2024

Complement runner configures the media worker as synapse.app.generic_worker instead of synapse.app.media_repository, which looks like it shouldn't work but apparently does #16826

Open

Add a --generate-only option to the Complement launcher. #16828

Merged

reivilibre closed this Jun 3, 2024

		sys.stderr.flush()


		def merge_into(dest: Any, new: Any) -> None:

Conversation

reivilibre commented Jan 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants