Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the configure_workers_and_start.py script used internally by Complement. #16803

Closed
wants to merge 20 commits into from
Closed
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
b39a50a
Remove obsolete `"app"` from worker templates
reivilibre Nov 16, 2023
a22eb7d
Convert worker templates into dataclass
reivilibre Nov 16, 2023
ba3b6a4
Use a lambda for the worker name rather than search and replace later
reivilibre Nov 16, 2023
67d4fc8
Collapse WORKERS_CONFIG by removing entries with defaults
reivilibre Nov 16, 2023
94a85b3
Convert listener_resources and endpoint_patterns to Set[str]
reivilibre Nov 16, 2023
26073fa
Tweak comments
reivilibre Nov 16, 2023
8b74639
Add `merge_into`
reivilibre Nov 16, 2023
f38297b
Remove special logic for adding stream_writers: just make it part of …
reivilibre Nov 16, 2023
7d8824e
Rename function to add_worker_to_instance_map given reduction of scope
reivilibre Nov 16, 2023
f49dbc7
Add `sharding_allowed` to the WorkerTemplate rather than having a sep…
reivilibre Nov 16, 2023
3bb21a9
Use `merge_into` when adding workers to the shared config
reivilibre Nov 16, 2023
fbafde8
Promote mark_filepath to constant
reivilibre Nov 16, 2023
321d359
Add a --generate-only option
reivilibre Nov 16, 2023
259a808
Docstring on WorkerTemplate
reivilibre Dec 6, 2023
3a46cf0
Fix comment and mutation bug on merge_worker_template_configs
reivilibre Dec 6, 2023
2f1d727
Update comment on `merged`
reivilibre Dec 6, 2023
ad4bb0e
Tweak `instantiate_worker_template`, both in name, description and va…
reivilibre Dec 6, 2023
2ff1de3
Newsfile
reivilibre Jan 10, 2024
29541fd
Move `stream_writers` to their own field in the WorkerTemplate
reivilibre Jan 17, 2024
c91ab4b
Remove `merge_into` and just have `merged` which copies inputs to avo…
reivilibre Jan 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions docker/configure_workers_and_start.py
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,31 @@ def flush_buffers() -> None:
sys.stderr.flush()


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
Merges `new` into `dest` with the following rules:

- dicts: values with the same key will be merged recursively
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

- lists: `new` will be appended to `dest`
- primitives: they will be checked for equality and inequality will result
in a ValueError

It is an error for `dest` and `new` to be of different types.
"""
if isinstance(dest, dict) and isinstance(new, dict):
for k, v in new.items():
if k in dest:
merge_into(dest[k], v)
else:
dest[k] = v
elif isinstance(dest, list) and isinstance(new, list):
dest.extend(new)
elif type(dest) != type(new):
raise TypeError(f"Cannot merge {type(dest).__name__} and {type(new).__name__}")
elif dest != new:
raise ValueError(f"Cannot merge primitive values: {dest!r} != {new!r}")


def convert(src: str, dst: str, **template_vars: object) -> None:
"""Generate a file from a template

Expand Down