-
Notifications
You must be signed in to change notification settings - Fork 307
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
Support per-process options & better per-role options #141
Open
metavida
wants to merge
16
commits into
seuros:master
Choose a base branch
from
haikulearning:multiple_config_files
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
...because presumably the developer is already smart enough to not put conflicting pidfile paths in their sidekiq config files.
Also update the default value to reflect the change made in seuros#116
For example ``` set :sidekiq_role, [:sidekiq_small, :sidekiq_big] set :sidekiq_small_config 'config/sidekiq/small.yml' set :sidekiq_big_processes 2 set :sidekiq_big_config ['config/sidekiq/first_big.yml', 'config/sidekiq/second_big.yml'] ```
Example: ``` set :sidekiq_role, [:web, :sidekiq_background] set :sidekiq_web_config, 'config/sidekiq_web.yml' set :sidekiq_background_config, 'config/sidekiq_bg.yml' ```
Also mark the sidekiq_options_per_process option as deprecated, since a user can simply set sidekiq_options to an Array
...instead of reinventing the wheel. Also, add more explicit documentation about the hack we have in place for backwards compatability.
While I think it's somewhat unlikely for a developer to want per-process env values, it's virtually free to add, so why not?
...and group options based on whether they're passed as options to sidekiq vs whether they're used to configure how capistrano behaves
This applies when stopping sidekiq
PING |
Ping |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note, this pull builds on top of pull #140, so includes all of its commits.
Per-process Options
Previously, if you wanted to specify different options for multiple sidekiq processes, you'd use
:sidekiq_options_per_process
. However, because pull #140 relies knowing the path in the:sidekiq_config
, I needed to be able to specify a different:sidekiq_config
value for each process.With this pull request, it's now possible to pass an Array of values to most of the sidekiq options. Any option with a non-Array value will be applied to all processes.
An example from the README
In this example the first process will start with the following options:
And the second sidekiq process will start with the following options:
Better Per-role Options
Previously, if a user provided an Array of
:sidekiq_role
values, they could customize the:sidekiq_processes
number per-role. However, I needed support for a different number of workers and various other config differences between roles.With this pull request, it's now possible to set per-role options for most of the sidekiq options. If both a non-role-specific value and a role-specific value are provided, the role-specific value will be preferred. Also note that when using a role that does not contain the prefix
:sidekiq_
, the option names still retain the:sidekiq_
prefix to avoid potential naming conflicts with other caistrano plugins.An example from the README
In this example the web1 host will have one process with the following options:
The bg1 host will have two processes. The first will have the following options:
The second sidekiq process on bg1 will start with the following options: