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

Update the sidekiq queue config #458

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

andyduong1920
Copy link
Member

@andyduong1920 andyduong1920 commented Aug 7, 2023

What happened 👀

Add the queues with Rails env prefix in order to allow the Sidekiq job get processed.

Insight 📝

The new default Rails generate had the config.active_job.queue_name_prefix = Rails.env which will require the Sidekiq queue to include these env prefixes.

Proof Of Work 📹

Used in client projects

Copy link
Contributor

@sanG-github sanG-github left a comment

Choose a reason for hiding this comment

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

I'm just curious if there are any benefits from this prefix since we seem never to run our application with both environments on the same machine. 🤔

config.active_job.queue_name_prefix = nil

@andyduong1920
Copy link
Member Author

I'm just curious if there are any benefits from this prefix since we seem never to run our application with both environments on the same machine. 🤔

config.active_job.queue_name_prefix = nil

The only benefit I can see is it allow us to have different setup per env like

  • development_default: 1
  • production_default: 3

@olivierobert
Copy link
Contributor

@sanG-github comment makes sense as the use case it covers does not exists for us as all environments are isolated. @Goose97 brought this into his IC but now it does not make sense to upstream this IMO 😅

@andyduong1920
Copy link
Member Author

@sanG-github @olivierobert But by default, the Rails generator will set the queue_name_prefix as Rails.env in the config/application.rb.

    # Prefix the queue name of all jobs with Rails ENV
    config.active_job.queue_name_prefix = Rails.env

So if we do not modify the queue here, the new application generated from our template won't work, and it requires the user to manually update the config/sidekiq.yml.

Incase we do not need this PR, we would need to open another PR in our template to - update the config.active_job.queue_name_prefix in config/application.rb to nil

    # Prefix the queue name of all jobs with Rails ENV
    config.active_job.queue_name_prefix = nil

cc @malparty @Goose97

@malparty
Copy link
Member

@olivierobert @sanG-github to clarify:

These new queue names (with env prefix) are the default generated by the rails new command.

So there are 2 options:

  • Keep the names with the env prefix, with no deviation from a standard Rails project (this PR suggestion)
  • Edit our template so that it will edit the config/application.rb file generated by rails new command to replace these lines:
    # Prefix the queue name of all jobs with Rails ENV
    config.active_job.queue_name_prefix = Rails.env

In my opinion, it's better to reduce the deviation with Rails defaults (less code, less maintenance to handle). So I recommend moving forward with this PR 👍

config/sidekiq.yml Outdated Show resolved Hide resolved
@andyduong1920 andyduong1920 force-pushed the feature/add-queue-with-Rails-env-prefix branch from a8868a7 to 0f52648 Compare September 14, 2023 08:47
@malparty malparty added this to the 5.9.0 milestone Sep 14, 2023
@sanG-github
Copy link
Contributor

@andyduong1920

Please help to resolve the failing CI, then we're good to go!!! 🚀

@malparty malparty force-pushed the feature/add-queue-with-Rails-env-prefix branch from 0f52648 to 166cd3e Compare September 19, 2023 02:29
@malparty
Copy link
Member

@andyduong1920 I just rebased that PR onto develop, that should fix the failing test 😇

@andyduong1920
Copy link
Member Author

@andyduong1920 I just rebased that PR onto develop, that should fix the failing test 😇

Thank you 🚀

@malparty malparty added this pull request to the merge queue Sep 20, 2023
Merged via the queue into develop with commit 34b8142 Sep 20, 2023
5 checks passed
@malparty malparty deleted the feature/add-queue-with-Rails-env-prefix branch September 20, 2023 07:06
@malparty malparty mentioned this pull request Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants