-
-
Notifications
You must be signed in to change notification settings - Fork 280
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 enqueue_after_transaction_commit? for Rails 7.2 compatibility #777
base: main
Are you sure you want to change the base?
Conversation
and the default for enqueue_after_transaction_commit? is true - see https://github.com/rails/rails/pull/51426/files#diff-572a695cc044413e9d509392a9230f9fbe8a039518f3e438a3c88f2b6a259480
Just realised that the problem with inheriting from Alternatively we could just use the initial commit or even remove the test. Any advice appreciated. |
I suspect removing the test probably makes for the simplest solution. Let me do that. |
and it complicates testing agaainst rails versions less than 7.2
It looks like ActiveJob might be changing the way it is handling enqueue_after_transaction_commit rails/rails#53336 and rails/rails#52675 |
hmm reading through https://github.com/rails/rails/pull/53375/files and I'm a little stumped on the recommended path for Shoryuken users. For rails 8 will not be an issue. For rails 7.2 should be set the configuration never?
|
By inheriting from |
Might be a good moment to transition to AWS Rails SDK that adds their own maintained SQS adapter for ActiveJob. I already tested in one app in production and works fine, although it's a slightly different approach, where jobs are executed in a running Rails app, not a separate worker process. https://github.com/aws/aws-sdk-rails Gemfile
config/aws_sqs_active_job.yml
config/environments/production.rb
|
@jpawlyn instead of inheriting, you could maybe |
@jeropaul as a temporary solution before Rails 8, you can monkey patch Shoryuken with this in the shoryuken initializer file for example ( module ActiveJob
module QueueAdapters
class ShoryukenAdapter
def enqueue_after_transaction_commit?
false
end
end
end
end |
I have been using the aws gem for well over a year "aws rails sdk". It does have some downside that this gem doesn't. mainly around running jobs on a worker outside of AWS Lambda. In our situation we have an event based application with many queues, one per worker. The AWS gem forces us to choose between, severly over provisioning the workers, or underutilizing the workers. This is because you can not setup a config where you want say 20 job max concurrently running. you have to choose how many queues essentially are running on the machine as a process will only run one queue at a time and hoping they all dont peak at the same time. This can lead to accidently overloading your machine in peak times, or just wasting $$ on resources if you over provision. |
@SteffanPerry , true, it's a different architecture. It may evolve towards replacing shoryuken but for now it's not a 1-1 drop-in replacement, I also still stick to Shoryuken in production. |
For Rails 7.2 there is a new method
enqueue_after_transaction_commit?
required on an ActiveJob adapter class - see:https://github.com/rails/rails/blob/dfd1e951aa1aeef06c39fffb2994db8a8fa1914f/activejob/lib/active_job/queue_adapters/abstract_adapter.rb#L10-L16 and https://api.rubyonrails.org/classes/ActiveJob/QueueAdapters/AbstractAdapter.html#method-i-enqueue_after_transaction_commit-3F