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 enqueue_after_transaction_commit? for Rails 7.2 compatibility #777

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

Conversation

jpawlyn
Copy link

@jpawlyn jpawlyn commented Sep 19, 2024

@jpawlyn
Copy link
Author

jpawlyn commented Sep 19, 2024

Just realised that the problem with inheriting from ActiveJob::QueueAdapters::AbstractAdapter is that the test will only pass with active job 7.2 but not sure how to do update the test to be conditional on the active job version.

Alternatively we could just use the initial commit or even remove the test. Any advice appreciated.

@jpawlyn
Copy link
Author

jpawlyn commented Sep 30, 2024

Just realised that the problem with inheriting from ActiveJob::QueueAdapters::AbstractAdapter is that the test will only pass with active job 7.2 but not sure how to do update the test to be conditional on the active job version.

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
@FinnIckler
Copy link

FinnIckler commented Oct 17, 2024

It looks like ActiveJob might be changing the way it is handling enqueue_after_transaction_commit rails/rails#53336 and rails/rails#52675
So this might be unnecessary soon. Still think this would be nice to have in the interim

@jeropaul
Copy link

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?

    Rails.application.config.active_job.enqueue_after_transaction_commit = :never

@jpawlyn
Copy link
Author

jpawlyn commented Oct 28, 2024

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?

    Rails.application.config.active_job.enqueue_after_transaction_commit = :never

By inheriting from ActiveJob::QueueAdapters::AbstractAdapter we should be good I think for Rails 7.2.

@phstc
Copy link
Collaborator

phstc commented Oct 29, 2024

@jpawlyn I can no longer maintain this project. I updated the README with additional info 946ed1d

@januszm
Copy link

januszm commented Nov 4, 2024

@jpawlyn I can no longer maintain this project.

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

gem 'aws-sdk-rails', '~> 4'

config/aws_sqs_active_job.yml

queues:
  default: "https://sqs.us-east-2.amazonaws.com/ACCOUNT_ID/QUEUE_NAME-<%= Rails.env %>-default.fifo"

config/environments/production.rb

config.active_job.queue_adapter = :sqs

@januszm
Copy link

januszm commented Nov 4, 2024

@jpawlyn instead of inheriting, you could maybe include ActiveJob::Enqueuing if it makes things simpler?

@januszm
Copy link

januszm commented Nov 7, 2024

@jeropaul as a temporary solution before Rails 8, you can monkey patch Shoryuken with this in the shoryuken initializer file for example (config/initializers/shoryuken.rb or wherever you put your initialization code)

module ActiveJob
  module QueueAdapters
    class ShoryukenAdapter
      def enqueue_after_transaction_commit?
        false
      end
    end
  end
end

@SteffanPerry
Copy link

SteffanPerry commented Nov 8, 2024

@jpawlyn I can no longer maintain this project.

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

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.

@januszm
Copy link

januszm commented Nov 8, 2024

@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.

@jpawlyn
Copy link
Author

jpawlyn commented Dec 2, 2024

@jpawlyn instead of inheriting, you could maybe include ActiveJob::Enqueuing if it makes things simpler?

Not sure but interested to know more @januszm

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.

6 participants