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

When using synctl with workers, don't start the main synapse automatically #2774

Merged
merged 3 commits into from Jan 12, 2018
Merged

When using synctl with workers, don't start the main synapse automatically #2774

merged 3 commits into from Jan 12, 2018

Conversation

erikjohnston
Copy link
Member

No description provided.

@erikjohnston erikjohnston changed the base branch from master to develop January 11, 2018 11:28
@erikjohnston erikjohnston assigned richvdh and unassigned richvdh Jan 11, 2018
richvdh
richvdh approved these changes Jan 11, 2018
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

synapse/app/synctl.py Outdated
assert worker_daemonize, "In config %r: expected '%s' to be True" % (
worker_configfile, "worker_daemonize")
worker_cache_factor = worker_config.get("synctl_cache_factor")
workers.append(Worker(
Copy link
Member

Choose a reason for hiding this comment

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

we can share this between the two branches provided you initialise worker_cache_factor correctly.

@richvdh richvdh assigned erikjohnston and unassigned richvdh Jan 11, 2018
@erikjohnston
Copy link
Member Author

Right, sorry, I managed to mess up testing this. Two sticking points:

  1. synapse.app.homeserver doesn't look at worker_pidfile etc
  2. There's a few places where we assume if worker_app is None then we're a worker.

For 1 I've made synctl complain if it sees worker_ config and for 2 I've canonicalised worker_app to be None if its the main process

@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston Jan 12, 2018
@richvdh
Copy link
Member

richvdh commented Jan 12, 2018

I can't help thinking it would be better if synapse.app.homeserver did look at the worker_* options?

@erikjohnston
Copy link
Member Author

I can't help thinking it would be better if synapse.app.homeserver did look at the worker_* options?

Yes, or even to not have the duplication between worker_pidfile and pidfile params, but I think that ends up with quite a bit of synapse fiddling (which I'm not enthusiastic about dong right now)

richvdh
richvdh approved these changes Jan 12, 2018
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

mmkay then

@erikjohnston erikjohnston merged commit fefeb0a into develop Jan 12, 2018
@erikjohnston erikjohnston deleted the erikj/synctl branch January 18, 2018 13:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@richvdh richvdh richvdh approved these changes

Assignees

@richvdh richvdh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@erikjohnston @richvdh