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

Possible regressions in Sidekiq 5.2.4 #4075

Closed
geoffharcourt opened this issue Jan 8, 2019 · 8 comments
Closed

Possible regressions in Sidekiq 5.2.4 #4075

geoffharcourt opened this issue Jan 8, 2019 · 8 comments

Comments

@geoffharcourt
Copy link
Contributor

geoffharcourt commented Jan 8, 2019

Ruby version: 2.5.3
Sidekiq / Pro / Enterprise version(s):
5.2.4 / 4.04 / 1.7.2

In 5.2.4 we saw two issues that I believe may be caused by the refactoring in this recent commit: (EDIT: it was a different recent commit, updated here)
1d83555

The issues are:

  1. workers with no declared queues via the CLI only process default, ignoring queues declared in the config YAML. Workers with queues declared as CLI arguments pick up those queues.
  2. The RAILS_MAX_THREADS environment variable appears to be suddenly taking precedence over the concurrency key in our configuration file. We have our app set to 3 and our SIDEKIQ_CONCURRENCY=15.

When we revert to 5.2.3, both of these issues resolve themselves. If we override RAILS_MAX_THREADS for our worker process at the CLI with RAILS_MAX_THREADS=15, we see the threads change to 15.

I'm still working up a failing test, but wanted to post this information as I'm working this out.

Please include your initializer and any error message with the full backtrace.
Our config/sidekiq.yml:

---
verbose: true
concurrency: <%= ENV.fetch("SIDEKIQ_CONCURRENCY", 5) %>
timeout: 25
queues:
  - [interims, 15]
  - [gc_assignments, 12]
  - [high_priority, 10]
  - [mailers, 10]
  - [toggle_notifications, 7]
  - [searchkick, 6]
  - [default, 5]
  - [low_priority, 2]

Environment settings:

RAILS_MAX_THREADS=3
SIDEKIQ_RAILS_MAX_THREADS=16
SIDEKIQ_CONCURRENCY=15

Procfile line for Heroku:

worker: NEW_RELIC_AGENT_ENABLED=true MALLOC_ARENA_MAX=2 PG_DB_CONNECTION_POOL=${SIDEKIQ_RAILS_MAX_THREADS} bundle exec sidekiq

(We have a second worker process which only processes mailers and high_priority that didn't only process default after the version bump.)

@geoffharcourt
Copy link
Contributor Author

geoffharcourt commented Jan 8, 2019

I think this change here:
1d83555#diff-411201f0104d91346964914342a07cefL372

and here:
1d83555#diff-411201f0104d91346964914342a07cefR235

Results in the configuration file in a default location being ignored entirely if it's not declared with the -C path/to/sidekiq.yml argument (opts, which would explain the loss of non-default queues and our concurrency setting. The tests cover -C and -r arguments, but I think there's missing coverage for neither option being specified.

@viraptor
Copy link

viraptor commented Jan 8, 2019

Confirming on 5.2.4 / 4.04 combination. After deployment no queues were processed with no obvious logs for any failures. Queues are defined in the config, so I'm likely facing issue 1.

@geoffharcourt
Copy link
Contributor Author

I'm having trouble writing a failing test as @cli.parse(%w[sidekiq]) (running the executable with no arguments) causes the test to end immediately and exit with an error, but I am more confident now that this is the problem. The workaround we're using is to add -C path/to/sidekiq.yml to your CLI command.

@mperham
Copy link
Collaborator

mperham commented Jan 8, 2019

I suspect this comment is referring to this regression #4054 (comment)

@mperham
Copy link
Collaborator

mperham commented Jan 8, 2019

Environment should take precedence over config file.

@mperham
Copy link
Collaborator

mperham commented Jan 8, 2019

I'm having trouble writing a failing test as @cli.parse(%w[sidekiq]) (running the executable with no arguments) causes the test to end immediately and exit with an error, but I am more confident now that this is the problem. The workaround we're using is to add -C path/to/sidekiq.yml to your CLI command.

CLI requires either the working directory to be a Rails app or -r to be specified.

@greysteil
Copy link

I'm pretty sure the default queue issue is due to a change in how the absence of a :require argument is handled. I have a failing test for it and will have a PR up in a moment.

@Tensho
Copy link
Contributor

Tensho commented Jan 8, 2019

Fixed in #4077

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants