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

Add GoodJob as Active Job backend #47

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ellery-nava
Copy link
Contributor

Ticket

Changes

  • Add GoodJob gem and migration to install
  • Update notification service to deliver later

Context for reviewers

Background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers.

Testing

  • Changes tested as part of the pfml-starter-kit-app repo

Copy link
Contributor

@rocketnova rocketnova left a 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"
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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!

Copy link
Contributor

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?

Comment on lines +79 to +80
# GoodJob was picked as a default because it uses Postgres (already a part of this template)
# and has good adoption and maintenence.
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, thanks!

app-rails/config/environments/production.rb Outdated Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.

Recommend default queuing backend for Active Job
2 participants