Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Support running multiple media repos. #7706

Merged
merged 4 commits into from
Jun 17, 2020
Merged

Conversation

erikjohnston
Copy link
Member

This requires a new config option to specify which media repo should be responsible for running background jobs to e.g. clear out expired URL preview caches.

This is a bit of an icky way of doing it, though I think somewhat consistent with how we do worker configs currently. There are a couple of other options here:

  1. Use distributed lock based on Redis, this is a bit difficult due to redis not having native support for this (the documentation suggests using a thing called Redlock, but the fact that Kleppman thinks its unsound is not exactly confidence inspiring). An argument could be made that we're probably will need to do something like this at some point though.
  2. Change the way we do our config to instead list which instances are responsible for media operations in the main config and chose the first to be responsible for running the background jobs. This has the advantage that a) its impossible to mess up and b) its similar to how we specify which worker is responsible for event persistence.

TBH though, it feels like we shouldn't block this behind trying to implement a distributed lock or changing how we configure workers

This requires a new config option to specify which media repo should be
responsible for running background jobs to e.g. clear out expired URL
preview caches.
@erikjohnston erikjohnston force-pushed the erikj/shard_media_repo branch from fb8c546 to 74295f3 Compare June 16, 2020 10:13
@erikjohnston erikjohnston requested a review from a team June 16, 2020 10:34
@clokep
Copy link
Member

clokep commented Jun 16, 2020

I think the implementation looks sane, I don't have an opinion about using a config option vs. other approaches.

@clokep clokep removed the request for review from a team June 16, 2020 17:48
@clokep
Copy link
Member

clokep commented Jun 16, 2020

Removing the review flag on this as Rich asked for some changes in #synapse-dev:matrix.org.

@erikjohnston
Copy link
Member Author

Ok, this now uses a scheme where you specify which instance runs the background jobs. Unfortunately, to support backwards compat we can't require it when using split out media repo and so if you don't specify the config option it'll still run on all of them.

@erikjohnston erikjohnston requested a review from a team June 17, 2020 10:21
docs/workers.md Outdated Show resolved Hide resolved
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

I think the backwards compatibility is OK since...you shouldn't have been running multiple in the past anyway, so hopefully people will read the docs on how to do it.

Co-authored-by: Patrick Cloke <[email protected]>
@erikjohnston erikjohnston merged commit b44bdd7 into develop Jun 17, 2020
@erikjohnston erikjohnston deleted the erikj/shard_media_repo branch June 17, 2020 13:13
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit 'b44bdd7f7':
  Support running multiple media repos. (#7706)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants