-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add sidekiq #291
Add sidekiq #291
Conversation
Current Code Coverage Percent of this PR:98.17 %Files having coverage below 100%
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohitjoshixyz Suggested some changes 👇
Also, Can't find the Procfile
for production, we'd want to add worker
command to that as well.
:queues: | ||
- default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:queues: | |
- default | |
queues: | |
- default | |
- mailers | |
- action_mailbox_routing | |
- action_mailbox_incineration | |
- active_storage_analysis | |
- active_storage_purge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this https://mikerogers.io/2019/06/06/rails-6-sidekiq-queues Are you suggesting adding them for the same reason? Are they used internally by active storage automatically or do we need more configuration? @shivamsinghchahar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly 💯
That's all we need, no additional configuration.
@rohitjoshixyz Another thing is sidekiq web UI, could we add that as well. |
Add Procfile for production Add more queues for active storage Remove sidekiq for test env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohitjoshixyz Just some minor changes, Rest LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Notion card
https://www.notion.so/Add-sidekiq-for-background-job-processing-0f2e618cda454d6980db0826cf202f8d
Summary
Followed: https://www.bigbinary.com/learn-rubyonrails-book/background-job-processing-using-sidekiq
Verified queuing is working by creating a job that puts "Sidekiq job executed" into the console.
Updated the readme to mention installing Redis.
Preview
Type of change
Please delete options that are not relevant.
not work as expected)
How Has This Been Tested?
Checklist: