8000 Upstream Active Job adapter by zzak · Pull Request #1906 · resque/resque · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Upstream Active Job adapter #1906

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Upstream Active Job adapter #1906

wants to merge 11 commits into from

Conversation

zzak
Copy link
@zzak zzak commented Sep 19, 2024

Fixes #1905

Rails wants to slim down the number of adapters it ships with.

This approach is not dissimilar from que-rb/que#403, but we have included all of the tests from Active Job to ensure it remains stable.

Not all of the tests are necessary, we can trim down the tests for non-resque adapters for example. But I wanted to start with a clean base, and find a pattern that can be applied to all of the adapters we want to extract.

The reason for the extension and const_remove is that the class name serialized for the job cannot change, otherwise we risk losing jobs.

In the future, after the adapter is removed downstream, you should be able to version guard that part at least. And eventually if you so choose, rename the constant, assuming that you give users ample time to migrate their queues but that seems unnecessary.

/cc rails/rails#52929

@zzak
Upstream Active Job adapter
a888e83 
Rails wants to slim down the number of adapters it ships with.

This approach is not dissimilar from que-rb/que#403, but we have
included all of the tests from Active Job to ensure it remains stable.

Not all of the tests are necessary, we can trim down the tests for
non-resque adapters for example. But I wanted to start with a clean
base, and find a pattern that can be applied to all of the adapters we
want to extract.

The reason for the extension and `const_remove` is that the class name
serialized for the job cannot change, otherwise we risk losing jobs.

In the future, after the adapter is removed downstream, you should be
able to version guard that part at least. And eventually if you so
choose, rename the constant, assuming that you give users ample time to
migrate their queues but that seems unnecessary.
This was referenced Sep 19, 2024
Merged
Open
@PatrickTulskie
Copy link
Contributor

@zzak Thanks for taking a crack at this. Admittedly, even though I use a lot of Rails and Resque in production, none of them use ActiveJob... never felt the need for the abstraction layer when we could just go direct to Resque.

With that in mind, can you explain this bit?

The reason for the extension and const_remove is that the class name serialized for the job cannot change, otherwise we risk losing jobs.

I gotta spend some time noodling the ramifications here and just make sure that by merging this in and okaying the downstream removal, we're not going to cause some pain for folks when they do an update. Probably wind up being a major Resque release to the say the least.

@zzak
Copy link
Author
zzak commented Sep 27, 2024 •
edited
Loading

@PatrickTulskie Thanks for your reply!

The goal would be to deprecate the adapter in Rails and remove it in the next release, given the upstream (resque in this case) is willing to take on the adapter. Then Rails can focus on a much leaner set of adapters to maintain and fewer dependencies which speeds things up like CI and save contributors time.

I've set things up to run on Rails main, so we can still know about breaking changes early, ideally there are none for the adapters. They are stable and that is another reason for this separation.

But I understand if you're not interested, have no use for Active Job in your work. I am happy to help maintain it if you'd like to add me as a contributor. I can also slim things down if this adds too much overhead, like to CI and such.

we're not going to cause some pain for folks when they do an update

This is exactly my goal, to avoid making any breaking changes other than a version bump in a lock file. You can see in sidekiq/sidekiq#6434 where I've demonstrated this.

To explain that comment about the constant naming, here is an example of two jobs queued up using different classes:

127.0.0.1:6379> LRANGE resque:queue:default 0 -1
1) "{\"class\":\"ActiveJob::QueueAdapters::ResqueAdapter::JobWrapper\",\"args\":[{\"job_class\":\"AddJob\",\"job_id\":\"8ba6c16c-5aca-4238-9663-78de9c3316bf\",\"provider_job_id\":null,\"queue_name\":\"default\",\"priority\":null,\"arguments\":[2,40],\"executions\":0,\"exception_executions\":{},\"locale\":\"en\",\"timezone\":\"UTC\",\"enqueued_at\":\"2024-09-18T23:15:26.238420118Z\",\"scheduled_at\":null}]}"
2) "{\"class\":\"Resque::ActiveJob::Adapter::JobWrapper\",\"args\":[{\"job_class\":\"AddJob\",\"job_id\":\"9d47ac9f-396c-412c-bec5-549eba352b05\",\"provider_job_id\":null,\"queue_name\":\"default\",\"priority\":null,\"arguments\":[2,40],\"executions\":0,\"exception_executions\":{},\"locale\":\"en\",\"timezone\":\"UTC\",\"enqueued_at\":\"2024-09-18T23:15:59.575453367Z\",\"scheduled_at\":null}]}"

And the result of trying to process that job with an unpatched version of the adapter:

[Resque::Job] Resque::Job ({"class"=>"Resque::ActiveJob::Adapter::JobWrapper", "args"=>[{"job_class"=>"AddJob", "job_id"=>"543f1892-7898-43c0-ac1a-4e260521ff3d", "provider_job_id"=>nil, "queue_name"=>"default", "priority"=>nil, "arguments"=>[20, 39, "from patched"], "executions"=>0, "exception_executions"=>{}, "locale"=>"en", "timezone"=>"UTC", "enqueued_at"=>"2024-09-19T01:03:30.431195554Z", "scheduled_at"=>nil}]}) failed
uninitialized constant Resque::ActiveJob
/home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/resque-2.6.0/lib/resque.rb:98:in `block in constantize'
/home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/resque-2.6.0/lib/resque.rb:92:in `each'
/home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/resque-2.6.0/lib/resque.rb:92:in `constantize'
/home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/resque-2.6.0/lib/resque/job.rb:59:in `constantize'
/home/zzak/code/apps/TestApp_aj_ex_main/config/initializers/active_job.rb:40:in `perform'

So the naming of the constant matters here if we are to avoid any breaking change. This way a Rails app can update to a version of Resque with the adapter and continue processing jobs happily, or in the middle of a roll out where both versions are running, and same for if a rollback is made but the queue still has new jobs in it. That should all be fine here.

@PatrickTulskie
Copy link
Contributor

@zzak Awesome thanks for that explanation. Also yeah, totally understand where DHH and the team are coming from and I think bringing the adapter into resque is the right thing to do here, especially given most people probably also use it with Rails. I just wanted to get some more information before moving forward and make sure we're all on the same page.

At my day job, I've been asking around and found a single app that is using the Resque adapter with ActiveJob so at least I have a real world app to test with before committing to this. Probably will prep an internal build in the next week or so, and if our cut over looks good I'll feel pretty good about merging this in. I've been going back and forth and I think this is probably major release worthy.

In preparation for that, I also started going through other PRs that have been languishing and trying to see if there is anything else that should get done before that major release. I'll try to get it done before Rails 8 but I can't make any promises at the moment.

@dixpac dixpac mentioned this pull request Oct 6, 2024
Merged
xjunior
xjunior reviewed Oct 21, 2024
View reviewed changes
lib/resque/active_job_extension.rb Outdated
@@ -0,0 +1,54 @@
# frozen_string_literal: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this just be in lib/active_job/queue_adapters/resque_adapter.rb instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Based on the class name and namespace, it should probably live there.

PatrickTulskie
PatrickTulskie reviewed Oct 24, 2024
View reviewed changes
Copy link
Contributor
@PatrickTulskie PatrickTulskie left a comment

Choose a reason for hiding this comment

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

Just gave this a quick pass and I agree with @xjunior that the actual adapter probably needs to move. The rest looks good to me though.

lib/resque/active_job_extension.rb Outdated
@@ -0,0 +1,54 @@
# frozen_string_literal: true
Copy link
Contributor

Choose a reason for hiding this comment

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

8000

Agreed. Based on the class name and namespace, it should probably live there.

zzak added 5 commits November 1, 2024 12:41
@zzak
Move AJ adapter
60ae102
@zzak
Rails main is Ruby 3.2 and greater
408ba5b
@zzak
Fix circular require from Rails AJ adapter
13e6517 
Similar to sidekiq/sidekiq#6477.

This resulted in a circular require, which will fail in Rails CI.

```
CI=1 bundle exec rake test:integration:resque
/home/zzak/.rbenv/versions/3.3.5/bin/ruby -w -I"lib:test" /home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb "test/integration/queuing_test.rb"
Using resque
/home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75: warning: /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75: warning: loading in progress, circular require considered harmful - /home/zzak/code/rails/activejob/lib/active_job/queue_adapters/resque_adapter.rb
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in  `<main>'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in  `select'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:21:in  `block in <main>'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `block (2 levels) in replace_require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `require'
        from /home/zzak/code/rails/activejob/test/integration/queuing_test.rb:3:in  `<top (required)>'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `block (2 levels) in replace_require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `require'
        from /home/zzak/code/rails/activejob/test/helper.rb:13:in  `<top (required)>'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `block (2 levels) in replace_require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `require'
        from /home/zzak/code/rails/activejob/test/support/integration/helper.rb:16:in  `<top (required)>'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `block (2 levels) in replace_require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `require'
        from /tmp/d20241101-2349270-pkyv1o/dummy/config/environment.rb:5:in  `<top (required)>'
        from /home/zzak/code/rails/railties/lib/rails/application.rb:440:in  `initialize!'
        from /home/zzak/code/rails/railties/lib/rails/initializable.rb:60:in  `run_initializers'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:208:in  `tsort_each'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:229:in  `tsort_each'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:350:in  `each_strongly_connected_component'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:350:in  `call'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:350:in  `each'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:352:in  `block in each_strongly_connected_component'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:418:in  `each_strongly_connected_component_from'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:418:in  `call'
        from /home/zzak/code/rails/railties/lib/rails/initializable.rb:50:in  `tsort_each_child'
        from /home/zzak/code/rails/railties/lib/rails/initializable.rb:50:in  `each'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:424:in  `block in each_strongly_connected_component_from'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:434:in  `each_strongly_connected_component_from'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:425:in  `block (2 levels) in each_strongly_connected_component_from'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:353:in  `block (2 levels) in each_strongly_connected_component'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/tsort.rb:231:in  `block in tsort_each'
        from /home/zzak/code/rails/railties/lib/rails/initializable.rb:61:in  `block in run_initializers'
        from /home/zzak/code/rails/railties/lib/rails/initializable.rb:32:in  `run'
        from /home/zzak/code/rails/railties/lib/rails/initializable.rb:32:in  `instance_exec'
        from /home/zzak/code/rails/railties/lib/rails/engine.rb:645:in  `block in <class:Engine>'
        from /home/zzak/code/rails/railties/lib/rails/engine.rb:645:in  `each'
        from /home/zzak/code/rails/railties/lib/rails/engine.rb:646:in  `block (2 levels) in <class:Engine>'
        from /home/zzak/code/rails/railties/lib/rails/engine.rb:692:in  `load_config_initializer'
        from /home/zzak/code/rails/activesupport/lib/active_support/notifications.rb:212:in  `instrument'
        from /home/zzak/code/rails/railties/lib/rails/engine.rb:693:in  `block in load_config_initializer'
        from /home/zzak/code/rails/railties/lib/rails/engine.rb:693:in  `load'
        from /tmp/d20241101-2349270-pkyv1o/dummy/config/initializers/activejob.rb:2:in  `<main>'
        from /home/zzak/code/rails/activejob/test/support/integration/adapters/resque.rb:5:in  `setup'
        from /home/zzak/code/rails/activejob/lib/active_job/queue_adapter.rb:52:in  `queue_adapter='
        from /home/zzak/code/rails/activejob/lib/active_job/queue_adapters.rb:136:in  `lookup'
        from /home/zzak/code/rails/activejob/lib/active_job/queue_adapters.rb:136:in  `const_get'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/zeitwerk-2.7.1/lib/zeitwerk/core_ext/kernel.rb:34:in  `require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/bootsnap-1.18.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in  `require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `block (2 levels) in repl
8000
ace_require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `require'
        from /home/zzak/code/rails/activejob/lib/active_job/queue_adapters/resque_adapter.rb:3:in  `<main>'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/zeitwerk-2.7.1/lib/zeitwerk/core_ext/kernel.rb:34:in  `require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/bootsnap-1.18.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in  `require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `block (2 levels) in replace_require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `require'
        from /home/zzak/code/resque/lib/resque.rb:27:in  `<main>'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/zeitwerk-2.7.1/lib/zeitwerk/core_ext/kernel.rb:34:in  `require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/bootsnap-1.18.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:30:in  `require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `block (2 levels) in replace_require'
        from /home/zzak/.rbenv/versions/3.3.5/lib/ruby/3.3.0/bundled_gems.rb:75:in  `require'
```

Interestingly, I started by working on resque/resque-scheduler#795,
but after this change that fix no longer seems necessary. Although maybe
safe to include anyways, and that PR also fixed the CI.
@zzak
Same as 408ba5b for test-activejob-integration job
f56a3b6
@zzak
Exclude Ruby < 3.2 for Rails 8.0 as well
e009306
@zzak
Copy link
Author
zzak commented Nov 1, 2024

@PatrickTulskie Thanks for the review and your patience, I made some time to update the file path, fix a circular require warning, and get the build 🟢

Happy to squash the commits if you prefer that, but wanted to leave them so you can review each one if that's helpful.

xjunior
xjunior reviewed Nov 1, 2024
View reviewed changes
lib/resque/railtie.rb Outdated
initializer "resque.active_job" do
ActiveSupport.on_load(:active_job) do
require "active_job/queue_adapters/resque_adapter"
ActiveJob::Base.queue_adapter = :resque
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should do this. I would rather leave this as an application decision.

Copy link
Author

Choose a reason for hiding this comment

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

@xjunior Whoops, good catch! Fixed in 49786b2.

xjunior
xjunior reviewed Nov 1, 2024
View reviewed changes
Copy link
Contributor
@xjunior xjunior left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not a maintainer, but left a comment. I feel like making that decision to the application can lead to unexpected behavior on the application. For instance, what if resque is brought in by a third-party dep and that takes it over the queue_adapter?

Furthermore, the adapter is more often than not managed and switched depending on the runtime environment (test for tests, async or inline for development, resque for production).

@xjunior
Copy link
Contributor
xjunior commented Nov 2, 2024

:shipit:

@zzak zzak mentioned this pull request Jan 19, 2025
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xjunior xjunior xjunior left review comments

@PatrickTulskie PatrickTulskie PatrickTulskie left review comments

@iloveitaly iloveitaly Awaiting requested review from iloveitaly

Assignees
No one assigned
Labels
None yet
4C1F
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Active Job adapter
3 participants
@zzak @PatrickTulskie @xjunior
0