Introduce update_user_directory_on

[ShadowJonathan]

Fixes #10339

This changes the update_user_directory config variable to a update_user_directory_on variable, which more explicitly points or denotes which process is supposed to be working user directory updates. This defaults to the main process.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

Signed-off-by: Jonathan de Jong <[email protected]>

[ShadowJonathan]

Blocked on matrix-org/sytest#1173 to let sytest worker tests proceed correctly.

[ShadowJonathan]

I added backwards compatibility to this, this is unblocked now.

[turt2live]

I have not looked at the patch, but it'd be quite nice to keep the disabling side effect of the flag. In my use case, t2bot.io doesn't update the user directory at all and there is no worker for it, so the original intent of the setting was to save CPU/DB cycles.

I will admit that it's a use case with sample size 1 though.

[ShadowJonathan]

Technically it would still work, as no string can match False, so consider that a feature :)

[turt2live]

can it get documented as a feature please? 😇

Relying on magic behaviour isn't great.

[richvdh]

this is definitely much clearer and easier to follow now, thanks! A few notes though.

[richvdh]

right, I see the confusion here now. "not allowed to" sounds like I've tried to enable something invalid. Maybe s/not allowed to/not configured to/.

But I don't really understand why this is a thing we need to warn about. Why is it a problem if the updates are done on the main process?

Note that the current wording sounds very much like an error, indicating something that definitely won't work.

[ShadowJonathan]

Maybe s/not allowed to/not configured to/.

👍

Why is it a problem if the updates are done on the main process?

That's not the problem, the problem is is that the configuration does not appoint the local worker to do the updates.

The config can now appoint any worker to do this job by its name, so the job of the user directory worker isn't that unique anymore, but it's uniqueness is still supported by synapse.

Note that the current wording sounds very much like an error

That's because it is, the local worker has a "type" of being a designated user update worker, while the configuration does not allow it to do just that. This is more or less a warning that "your config, as-is, is contradictory"

The previous version exited the process with 1 when hitting this point, should that be re-introduced?

[richvdh]

Ugh, I remain hopelessly confused about what is going on here, and how it fixes the original issue (#10339). I have commented there.

[ShadowJonathan]

I'll be closing this PR in light of the retrospective at #10339 (comment), where it became clear this PR is the wrong solution to this problem.