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

ActiveJob: queue_adapter can be inherited #16992

Merged
merged 1 commit into from Mar 12, 2015

Conversation

Projects
None yet
@tamird
Contributor

tamird commented Sep 20, 2014

@tamird tamird changed the title from allow overriding sub job queue_adapter without affecting the base job or... to AJ::Base.queue_adapter improvements Sep 24, 2014

+ performed_jobs << {job: job.class, args: job.arguments, queue: job.queue_name}
+ job.perform_now
+ else
+ enqueued_jobs << {job: job.class, args: job.arguments, queue: job.queue_name}

This comment has been minimized.

@lukaszx0

lukaszx0 Sep 24, 2014

Member

We have 4 lines like this. Maybe let's return it from a method like job_hash(job) ?

@lukaszx0

lukaszx0 Sep 24, 2014

Member

We have 4 lines like this. Maybe let's return it from a method like job_hash(job) ?

This comment has been minimized.

@lukaszx0

lukaszx0 Sep 24, 2014

Member

Actually even more natural would be job.to_hash

@lukaszx0

lukaszx0 Sep 24, 2014

Member

Actually even more natural would be job.to_hash

This comment has been minimized.

@tamird

tamird Sep 24, 2014

Contributor

done

@tamird

tamird Sep 24, 2014

Contributor

done

+ performed_jobs << {job: job.class, args: job.arguments, queue: job.queue_name, at: timestamp}
+ job.perform_now
+ else
+ enqueued_jobs << {job: job.class, args: job.arguments, queue: job.queue_name, at: timestamp}

This comment has been minimized.

@lukaszx0

lukaszx0 Sep 24, 2014

Member

job_hash(job, at: timestamp)

@lukaszx0

lukaszx0 Sep 24, 2014

Member

job_hash(job, at: timestamp)

@lukaszx0 lukaszx0 added the activejob label Sep 24, 2014

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Sep 24, 2014

Member

The test adapter was made like this so we can run tests in parallel. @seuros you worked on this
Anyway if you are going to do this make a separate PR

Member

cristianbica commented Sep 24, 2014

The test adapter was made like this so we can run tests in parallel. @seuros you worked on this
Anyway if you are going to do this make a separate PR

require 'active_support/core_ext/string/inflections'
module ActiveJob
module QueueAdapter
extend ActiveSupport::Concern
+ included do
+ class_attribute :_queue_adapter, instance_accessor: false, instance_predicate: false

This comment has been minimized.

@cristianbica

cristianbica Sep 24, 2014

Member

Please rename _queue_adapter to something else _queue_adapter_class or even queue_adapter_class to be more clear

@cristianbica

cristianbica Sep 24, 2014

Member

Please rename _queue_adapter to something else _queue_adapter_class or even queue_adapter_class to be more clear

This comment has been minimized.

@tamird

tamird Sep 24, 2014

Contributor

It's not always a class, though. It may be a callable. I'll change the name to reflect that.

@tamird

tamird Sep 24, 2014

Contributor

It's not always a class, though. It may be a callable. I'll change the name to reflect that.

module ClassMethods
- mattr_reader(:queue_adapter) { ActiveJob::QueueAdapters::InlineAdapter }
+ def queue_adapter
+ if _queue_adapter.respond_to?(:call)

This comment has been minimized.

@cristianbica

cristianbica Sep 24, 2014

Member

Can you provide a use case where you need to determine the adapter in a proc? We should implement features when there are clear use cases for them (I think DHH's rule is at least 2)

@cristianbica

cristianbica Sep 24, 2014

Member

Can you provide a use case where you need to determine the adapter in a proc? We should implement features when there are clear use cases for them (I think DHH's rule is at least 2)

This comment has been minimized.

@tamird

tamird Sep 24, 2014

Contributor

Sure, I needed this today in production actually. In the case of switching from one job queue to another, it may be desirable to enqueue a percentage of jobs to say, sidekiq, while the rest go to resque. We're running a similar thing right now where the percentage is stored in a redis key, allowing us to ramp up dynamically, and roll back if problems are detected (without a deploy).

@tamird

tamird Sep 24, 2014

Contributor

Sure, I needed this today in production actually. In the case of switching from one job queue to another, it may be desirable to enqueue a percentage of jobs to say, sidekiq, while the rest go to resque. We're running a similar thing right now where the percentage is stored in a redis key, allowing us to ramp up dynamically, and roll back if problems are detected (without a deploy).

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Sep 24, 2014

Member

Here's the PR when we switch the test adapter to instance methods #16724 (diff)

Member

cristianbica commented Sep 24, 2014

Here's the PR when we switch the test adapter to instance methods #16724 (diff)

@tamird tamird referenced this pull request Sep 24, 2014

Merged

[ActiveJob] TestCase #16724

activejob/test/cases/test_helper_test.rb
@@ -7,6 +7,7 @@
class EnqueuedJobsTest < ActiveJob::TestCase
setup { queue_adapter.perform_enqueued_at_jobs = true }
+ teardown { queue_adapter.perform_enqueued_at_jobs = false }

This comment has been minimized.

@seuros

seuros Sep 24, 2014

Member

We don't need these teardowns. The test adapter sould to allow parallel test instantiated for every test

@seuros

seuros Sep 24, 2014

Member

We don't need these teardowns. The test adapter sould to allow parallel test instantiated for every test

This comment has been minimized.

@tamird

tamird Sep 24, 2014

Contributor

Not anymore, it's a singleton now. See existing discussion in this PR.

@tamird

tamird Sep 24, 2014

Contributor

Not anymore, it's a singleton now. See existing discussion in this PR.

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Sep 24, 2014

Member

Ok. So ... as discussed the ability to pass a proc to queue_adapter is still under discussion so I suggest to leave this PR with the initial scope of being able to override sub-jobs adapters and start a new PR with the proc thing.
On the other hand we need to document the current change so we need to add documentation to queue_adapter= and queue_adapter. What would be nicer is if you have some time to update the AJ guide :)

Member

cristianbica commented Sep 24, 2014

Ok. So ... as discussed the ability to pass a proc to queue_adapter is still under discussion so I suggest to leave this PR with the initial scope of being able to override sub-jobs adapters and start a new PR with the proc thing.
On the other hand we need to document the current change so we need to add documentation to queue_adapter= and queue_adapter. What would be nicer is if you have some time to update the AJ guide :)

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Sep 24, 2014

Member

Also you need to rebase before we're ready to merge

Member

cristianbica commented Sep 24, 2014

Also you need to rebase before we're ready to merge

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Sep 24, 2014

Member

I meant to rebase so you can squash your commits

Member

cristianbica commented Sep 24, 2014

I meant to rebase so you can squash your commits

@seuros

This comment has been minimized.

Show comment
Hide comment
@seuros

seuros Sep 24, 2014

Member

Then you should split them into multiple PRs.
I don't like the singleton conversion. we had it like that and changed it to instance . cc @jeremy

Member

seuros commented Sep 24, 2014

Then you should split them into multiple PRs.
I don't like the singleton conversion. we had it like that and changed it to instance . cc @jeremy

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Sep 24, 2014

Member

No need to squash or split it into multiple PRs.

Looks good to me 👍

Member

lukaszx0 commented Sep 24, 2014

No need to squash or split it into multiple PRs.

Looks good to me 👍

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Sep 25, 2014

Member

I'd keep TestAdapter as a class with a single instance rather than switching to using a class object as the adapter. Nice cleanup of the adapter internals in either case.

Member

jeremy commented Sep 25, 2014

I'd keep TestAdapter as a class with a single instance rather than switching to using a class object as the adapter. Nice cleanup of the adapter internals in either case.

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Oct 8, 2014

Member

@tamir this is the same as #16977 so we can close #16977, right?

Member

cristianbica commented Oct 8, 2014

@tamir this is the same as #16977 so we can close #16977, right?

@tamir

This comment has been minimized.

Show comment
Hide comment
@tamir

tamir Oct 8, 2014

I was erroneously ccd. the mail should have gone to @tamird i think.

On 08 Oct 2014, at 09:02, Cristian Bica notifications@github.com wrote:

@tamir this is the same as #16977 so we can close #16977, right?


Reply to this email directly or view it on GitHub.

tamir commented Oct 8, 2014

I was erroneously ccd. the mail should have gone to @tamird i think.

On 08 Oct 2014, at 09:02, Cristian Bica notifications@github.com wrote:

@tamir this is the same as #16977 so we can close #16977, right?


Reply to this email directly or view it on GitHub.

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Feb 23, 2015

Member

So:

  1. Having different queue_adapter per job class: I'm ok with it but I remember talking something about Module#prepend to make the code cleaner but we couldn't as Rails 4.2 supports ruby 1.9.3. Now we support ruby 2.2+ so we could use Module#prepend. Do you remember?
  2. Queue adapter as a callable. I still have my doubts on this as the usage of this feature will be pretty low and there is an alternative: you could temporary define a queue_adapter class method on your job and remove it after you're done with the migration
  3. I don't remember the reasoning for the TestAdapter thing but as @jeremy said to keep it this way let's not touch it for now.

So IMO you should create a new PR with the first commit (and it would be nice to have a cleaner implementation with Module#prepend) and for the last 2 let's wait for some more feedback.

Member

cristianbica commented Feb 23, 2015

So:

  1. Having different queue_adapter per job class: I'm ok with it but I remember talking something about Module#prepend to make the code cleaner but we couldn't as Rails 4.2 supports ruby 1.9.3. Now we support ruby 2.2+ so we could use Module#prepend. Do you remember?
  2. Queue adapter as a callable. I still have my doubts on this as the usage of this feature will be pretty low and there is an alternative: you could temporary define a queue_adapter class method on your job and remove it after you're done with the migration
  3. I don't remember the reasoning for the TestAdapter thing but as @jeremy said to keep it this way let's not touch it for now.

So IMO you should create a new PR with the first commit (and it would be nice to have a cleaner implementation with Module#prepend) and for the last 2 let's wait for some more feedback.

@tamir

This comment has been minimized.

Show comment
Hide comment
@tamir

tamir Feb 23, 2015

could you please take me off the cc!
Im not interested and the person this is intended for does not get the message!
please cc tamird

On 23 Feb 2015, at 21:48, Cristian Bica notifications@github.com wrote:

So:

  1. Having different queue_adapter per job class: I'm ok with it but I remember talking something about Module#prepend to make the code cleaner but we couldn't as Rails 4.2 supports ruby 1.9.3. Now we support ruby 2.2+ so we could use Module#prepend. Do you remember?
  2. Queue adapter as a callable. I still have my doubts on this as the usage of this feature will be pretty low and there is an alternative: you could temporary define a queue_adapter class method on your job and remove it after you're done with the migration
  3. I don't remember the reasoning for the TestAdapter thing but as @jeremy said to keep it this way let's not touch it for now.

So IMO you should create a new PR with the first commit (and it would be nice to have a cleaner implementation with Module#prepend) and for the last 2 let's wait for some more feedback.


Reply to this email directly or view it on GitHub.

tamir commented Feb 23, 2015

could you please take me off the cc!
Im not interested and the person this is intended for does not get the message!
please cc tamird

On 23 Feb 2015, at 21:48, Cristian Bica notifications@github.com wrote:

So:

  1. Having different queue_adapter per job class: I'm ok with it but I remember talking something about Module#prepend to make the code cleaner but we couldn't as Rails 4.2 supports ruby 1.9.3. Now we support ruby 2.2+ so we could use Module#prepend. Do you remember?
  2. Queue adapter as a callable. I still have my doubts on this as the usage of this feature will be pretty low and there is an alternative: you could temporary define a queue_adapter class method on your job and remove it after you're done with the migration
  3. I don't remember the reasoning for the TestAdapter thing but as @jeremy said to keep it this way let's not touch it for now.

So IMO you should create a new PR with the first commit (and it would be nice to have a cleaner implementation with Module#prepend) and for the last 2 let's wait for some more feedback.


Reply to this email directly or view it on GitHub.

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Feb 23, 2015

Member

@tamir github added you as a follower when I cc'ed you by mistake last year (sorry for that). You need to click on the unsubscribe button on the right sidebar to stop getting notifications about this.

Member

cristianbica commented Feb 23, 2015

@tamir github added you as a follower when I cc'ed you by mistake last year (sorry for that). You need to click on the unsubscribe button on the right sidebar to stop getting notifications about this.

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Feb 23, 2015

Member

@tamird I know :) but it's a similar implementation and you participated in the talk and I was asking you if you remember what were the implications of Module#prepend.

Member

cristianbica commented Feb 23, 2015

@tamird I know :) but it's a similar implementation and you participated in the talk and I was asking you if you remember what were the implications of Module#prepend.

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Feb 23, 2015

Member

I think the idea was not to have the auxiliary _queue_adapter. Would something like this gist work: https://gist.github.com/cristianbica/0e14926fb730e8cf2c59?

Member

cristianbica commented Feb 23, 2015

I think the idea was not to have the auxiliary _queue_adapter. Would something like this gist work: https://gist.github.com/cristianbica/0e14926fb730e8cf2c59?

@tamird tamird changed the title from AJ::Base.queue_adapter improvements to ActiveJob: queue_adapter can be inherited Feb 24, 2015

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Feb 24, 2015

+ # ClassMethods rather than `extend`ing it.
+ def self.included(klass)
+ klass.class_attribute :queue_adapter, instance_accessor: false, instance_predicate: false
+ klass.singleton_class.prepend(ClassMethods)

This comment has been minimized.

@jeremy

jeremy Mar 12, 2015

Member

Rather than use an attribute with the same name as public API, pick an internal name, e.g. :_queue, and

alias_method :queue_adapter, :_queue
def queue_adapter=(name_or_adapter_or_class) self._queue = interpret_adapter(name_or_adapter_or_class) end
@jeremy

jeremy Mar 12, 2015

Member

Rather than use an attribute with the same name as public API, pick an internal name, e.g. :_queue, and

alias_method :queue_adapter, :_queue
def queue_adapter=(name_or_adapter_or_class) self._queue = interpret_adapter(name_or_adapter_or_class) end

This comment has been minimized.

@tamird

tamird Mar 12, 2015

Contributor

yeah, this is similar to what I had before applying the second commit on this branch at @cristianbica's request. Would you like me to strip that commit off?

@tamird

tamird Mar 12, 2015

Contributor

yeah, this is similar to what I had before applying the second commit on this branch at @cristianbica's request. Would you like me to strip that commit off?

This comment has been minimized.

@jeremy

jeremy Mar 12, 2015

Member

👍

activejob/lib/active_job/test_helper.rb
+ test_adapter = ActiveJob::QueueAdapters::TestAdapter.new
+
+ @old_queue_adapters = (ActiveJob::Base.subclasses << ActiveJob::Base).select do |klass|
+ klass.respond_to?(:queue_adapter)

This comment has been minimized.

@jeremy

jeremy Mar 12, 2015

Member

Every ActiveJob::Base subclass will respond to :queue_adapter since it's a singleton method on Base

@jeremy

jeremy Mar 12, 2015

Member

Every ActiveJob::Base subclass will respond to :queue_adapter since it's a singleton method on Base

This comment has been minimized.

@tamird

tamird Mar 12, 2015

Contributor

derp, you're right. fixed

@tamird

tamird Mar 12, 2015

Contributor

derp, you're right. fixed

activejob/lib/active_job/test_helper.rb
+ [klass, klass.queue_adapter].tap do
+ klass.queue_adapter = test_adapter
+ end
+ end

This comment has been minimized.

@jeremy

jeremy Mar 12, 2015

Member

Since class attributes no longer inherit from superclass once they're written, assigning #queue_adapter on classes the didn't have it explicitly set will leak side effects between test cases (since changing a superclass attr won't be inherited by its subclass afterward)

@jeremy

jeremy Mar 12, 2015

Member

Since class attributes no longer inherit from superclass once they're written, assigning #queue_adapter on classes the didn't have it explicitly set will leak side effects between test cases (since changing a superclass attr won't be inherited by its subclass afterward)

This comment has been minimized.

@tamird

tamird Mar 12, 2015

Contributor

great catch, fixed

@tamird

tamird Mar 12, 2015

Contributor

great catch, fixed

activejob/lib/active_job/test_helper.rb
+ # only override explicitly set adapters, a quirk of `class_attribute`
+ klass.singleton_class.public_instance_methods(false).include?(:_queue_adapter)
+ end.map do |klass|
+ [klass, klass._queue_adapter].tap do

This comment has been minimized.

@jeremy

jeremy Mar 12, 2015

Member

Prefer to use the public-API reader?

@jeremy

jeremy Mar 12, 2015

Member

Prefer to use the public-API reader?

This comment has been minimized.

@tamird

tamird Mar 12, 2015

Contributor

fixed

@tamird

tamird Mar 12, 2015

Contributor

fixed

activejob/lib/active_job/test_helper.rb
@@ -16,7 +26,9 @@ def before_setup
def after_teardown
super
- ActiveJob::Base.queue_adapter = @old_queue_adapter
+ @old_queue_adapters.each do |(klass, adapter)|
+ klass._queue_adapter = adapter

This comment has been minimized.

@jeremy

jeremy Mar 12, 2015

Member

Can use the public writer method too

@jeremy

jeremy Mar 12, 2015

Member

Can use the public writer method too

This comment has been minimized.

@tamird

tamird Mar 12, 2015

Contributor

derp, fixed

@tamird

tamird Mar 12, 2015

Contributor

derp, fixed

`ActiveJob::Base#queue_adapter` is now a `class_attribute`
This allows different `queue_adapters` to be used in each `ActiveJob`
class heirarchy. Previously, all subclasses used a single global queue
adapter.

jeremy added a commit that referenced this pull request Mar 12, 2015

@jeremy jeremy merged commit 202c68c into rails:master Mar 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tamird tamird deleted the square:configurable-job-queue-adapter branch Mar 12, 2015

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Mar 12, 2015

Member

❤️ took a few months to get this merged :)

Member

cristianbica commented Mar 12, 2015

❤️ took a few months to get this merged :)

@trungpham

This comment has been minimized.

Show comment
Hide comment

👍

@rebyn

This comment has been minimized.

Show comment
Hide comment
@rebyn

rebyn Apr 13, 2015

Contributor

I'm not seeing this feature being mentioned in the Basics guide. Or we have an Advanced guide somewhere that I don't know :)?

I think this is cool and deserves a mention. Should I submit a PR or has anyone worked on this already?

Contributor

rebyn commented Apr 13, 2015

I'm not seeing this feature being mentioned in the Basics guide. Or we have an Advanced guide somewhere that I don't know :)?

I think this is cool and deserves a mention. Should I submit a PR or has anyone worked on this already?

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

@benoitongit

This comment has been minimized.

Show comment
Hide comment
@benoitongit

benoitongit Mar 7, 2016

👍
Really cool, thanks! Would be great to add it in the basic guide, it took some time to know it was possible.

👍
Really cool, thanks! Would be great to add it in the basic guide, it took some time to know it was possible.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Jul 21, 2017

Contributor

Is code merged in such that it's possible to have different queue adapters for different jobs? (and if so beginning in what Rails version)

I got here following a trail of links between issues and PRs here and in ActiveJob. And the disussion and statuses here make me think this is now possible. But I have no idea how to do it!

If I'm on the right track, could anyone post an example here? If I get an example, I will try to play with it and understand it and do a doc PR demo'ing it in the right place. (If it's already in the docs but I just haven't found it, would very much appreciate if you wanted to point me there!).

Contributor

jrochkind commented Jul 21, 2017

Is code merged in such that it's possible to have different queue adapters for different jobs? (and if so beginning in what Rails version)

I got here following a trail of links between issues and PRs here and in ActiveJob. And the disussion and statuses here make me think this is now possible. But I have no idea how to do it!

If I'm on the right track, could anyone post an example here? If I get an example, I will try to play with it and understand it and do a doc PR demo'ing it in the right place. (If it's already in the docs but I just haven't found it, would very much appreciate if you wanted to point me there!).

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Jul 21, 2017

Contributor

Nevermind, just found it in the guide! "You can also configure your backend on a per job basis." Okay! Great!

Contributor

jrochkind commented Jul 21, 2017

Nevermind, just found it in the guide! "You can also configure your backend on a per job basis." Okay! Great!

@steveburkett

This comment has been minimized.

Show comment
Hide comment
@steveburkett

steveburkett Oct 4, 2017

does anyone have a "patch" i can use in my rails 4.2 app to get this to work?

does anyone have a "patch" i can use in my rails 4.2 app to get this to work?

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