8000 when retrieving queue name, check for 'queue_name' which is how ActiveJob defines queue names by josh-m-sharpe · Pull Request #1713 · resque/resque · GitHub
[go: up one dir, main page]
Skip to content

when retrieving queue name, check for 'queue_name' which is how ActiveJob defines queue names#1713

Open
josh-m-sharpe wants to merge 1 commit intoresque:masterfrom
josh-m-sharpe:check_for_queue_as
Open

when retrieving queue name, check for 'queue_name' which is how ActiveJob defines queue names#1713
josh-m-sharpe wants to merge 1 commit intoresque:masterfrom
josh-m-sharpe:check_for_queue_as

Conversation

@josh-m-sharpe
Copy link
Contributor
@josh-m-sharpe josh-m-sharpe commented Apr 5, 2020

@rafaelfranca
Copy link
Member
rafaelfranca commented Apr 8, 2020

queue_as is how Active Job defines the queue, not how you get the value. Should not this being calling queued_name?

But either way, Resque should not know anything about Active Job.

Why do you think this change is necessary? The Active Job adapter already set the correct queue name to Resque. https://github.com/rails/rails/blob/855c9897eb98ea4d77ec5036d15cdd1764698838/activejob/lib/active_job/queue_adapters/resque_adapter.rb#L33

@josh-m-sharpe
Copy link
Contributor Author
josh-m-sharpe commented Apr 8, 2020 •
edited
Loading

Branch updated to use queue_name.

I needed this to allow resque-scheduler to correctly queue ActiveJobs: https://github.com/resque/resque-scheduler/blob/master/lib/resque/scheduler/server.rb#L149 - that plugin uses Resque.queue_from_class in a couple other places as well.

I suspect this fix is probably still necessary in this context given that resque-scheduler is a plugin for resque and not ActiveJob... probably??

@josh-m-sharpe
Copy link
Contributor Author
josh-m-sharpe commented Apr 8, 2020 •
edited
Loading

Yea, I think the issue (which has nothing to do with resque-scheudler) is that Resque.queue_from_class returns false for ActiveJob jobs. The expectation below would be 'default'

$ cat app/jobs/standard_job.rb
class StandardJob < ApplicationJob
  queue_as :default

  def perform(*args)
    sleep 0.1
  end
end

$ cat app/jobs/application_job.rb
class ApplicationJob < ActiveJob::Base
end

$ rc
Loading development environment (Rails 6.0.2.2)
[1] pry(main)> Resque.queue_from_class(StandardJob)
=> false

@rafaelfranca
Copy link
Member
rafaelfranca commented Apr 8, 2020

That makes sense. That happen because resque doesn't, and should not, know about Active Job. So Resque.queue_from_class(StandardJob) should return false.

Also, we can't determine the queue name for a subclass of ActiveJob::Base using the class. We need to use the instance.

That being said, in order to expose the queue for a job we will need to make significant changes to the Resque plugin in Active Job to wrap each job in a class like we do in the Sidekiq adapter.

@josh-m-sharpe josh-m-sharpe changed the title when retrieving queue name, check for 'queue_as' which is how ActiveJob defines queue names when retrieving queue name, check for 'queue_name' which is how ActiveJob defines queue names Apr 8, 2020
@josh-m-sharpe
Copy link
Contributor Author
josh-m-sharpe commented Apr 8, 2020

Also, we can't determine the queue name for a subclass of ActiveJob::Base using the class. We need to use the instance.

This PR is doing: klass.new.queue_name so I believe it is pulling the correct queue name.

Apologies, but I'm not sure I follow what you are saying here:

That being said, in order to expose the queue for a job we will need to make significant changes to the Resque plugin in Active Job to wrap each job in a class like we do in the Sidekiq adapter.

@josh-m-sharpe josh-m-sharpe mentioned this pull request Apr 8, 2020
Open
iloveitaly
iloveitaly requested changes Sep 16, 2021
View reviewed changes
Copy link
Contributor
@iloveitaly iloveitaly left a comment

Choose a reason for hiding this comment

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

@josh-m-sharpe Although I agree with rafaelfranca in principle—that resque shouldn't know about ActiveJob—I think it's a better UX to add some light bindings / support for ActiveJob given that's a very popular pattern.

@rafaelfranca Could you explain further why "we will need to make significant changes to the Resque plugin in Active Job"? I'm not seeing any issues here, but I don't have as much experience with ActiveJob.

lib/resque.rb
(klass.respond_to?(:queue) and klass.queue)
klass.instance_variable_get(:@queue) ||
(klass.respond_to?(:queue) && klass.queue) ||
(klass.respond_to?(:queue_name) && klass.new.queue_name)
Copy link
Contributor
@iloveitaly iloveitaly Sep 16, 2021

Choose a reason for hiding this comment

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

You could add more inline documentation here about what cases these different lines are handling? Even just a link to this PR in code would be helpful.

Copy link
Contributor
@iloveitaly iloveitaly Oct 18, 2021

Choose a reason for hiding this comment

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

@josh-m-sharpe quick reminder here! Would love to tidy up this PR and get it merged.

Uh oh!

There was an error while loading. Please reload this page.
Copy link
Contributor Author
@josh-m-sharpe josh-m-sharpe Oct 18, 2021

Choose a reason for hiding this comment

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

Feel free to tidy it up and get it merged!

Copy link
Contributor
@varyonic varyonic Sep 29, 2025

Choose a reason for hiding this comment

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

Thank you for this, I found it useful. Has it been superseded by another solution or shall I fork/rebase and resubmit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iloveitaly iloveitaly iloveitaly requested changes

+1 more reviewer

@varyonic varyonic varyonic left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@josh-m-sharpe @rafaelfranca @iloveitaly @varyonic
0