-
Notifications
You must be signed in to change notification settings - Fork 2
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 GoodJob as Active Job backend #47
base: main
Are you sure you want to change the base?
Conversation
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.
Generally looks great. Could you add an ADR for this into /docs/app-rails/decisions
? Thank you!
@@ -75,9 +75,13 @@ | |||
# Use a different cache store in production. | |||
# config.cache_store = :mem_cache_store | |||
|
|||
# Use a real queuing backend for Active Job (and separate queues per environment). | |||
# config.active_job.queue_adapter = :resque | |||
# config.active_job.queue_name_prefix = "app_rails_production" |
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.
Do you think we need a queue_name_prefix
?
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.
You have a lot more Rails expertise than I do so please LMK if this is the wrong line of thinking, but since jobs are queued using a table in the DB I don't think that we'd need to worry about a prefix since we won't be sharing the database across environments? I would think that the queue prefix might be more necessary if we were doing something like sharing a Redis instance across multiple environments, for example.
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.
since jobs are queued using a table in the DB I don't think that we'd need to worry about a prefix since we won't be sharing the database across environments?
Good point!
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.
After looking at this and thinking about it more, I think we should move this line:
config.active_job.queue_adapter = :good_job
out of the per-environment files and into /app-rails/config/application.rb
. What do you think? Good Jobs should definitely be used in the mock-production
environment. I'm not sure what it might do to the test environment, though. Thoughts?
# GoodJob was picked as a default because it uses Postgres (already a part of this template) | ||
# and has good adoption and maintenence. |
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.
This should get moved to an ADR and a link to that should be added here.
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.
Will do, thanks!
Co-authored-by: Rocket <[email protected]>
Ticket
Changes
Context for reviewers
Testing