Add job priorities to ActiveJob #19425

Merged
merged 1 commit into from Sep 26, 2015

Conversation

Projects
None yet
@wvengen
Contributor

wvengen commented Mar 20, 2015

The Active Job documentation mentions that some backends allow setting priority per job. This PR adds support for setting job-level priorites to Active Job.

  • Similar to queue_name, the priority can be set on the class level and can be overridden per job.
  • This only brings support to job-based priorities, not queue-based priorities. (Though the same syntax could be used, with the backend checking that the same priority is always used per queue.)
  • The default priority for delayed_job is 0, for que 100. To keep backward-compatibility, the job priority is nil by default, but this can be overridden by setting default_priority. Both backends use the default priority when it is nil.
  • Backends that don't support priorities will silently ignore any priority setting. Perhaps an exception could be thrown (like scheduled jobs with the inline backend), but as most backends will need to do that, I guess some refactoring is needed. Since this is my first contribution, I've kept it with this for now.

@seuros seuros added the activejob label Mar 20, 2015

@seuros seuros added this to the 5.0.0 milestone Mar 20, 2015

@avokhmin

This comment has been minimized.

Show comment
Hide comment
@avokhmin

avokhmin Aug 3, 2015

Contributor

👍 , @wvengen can you do a rebase?

Contributor

avokhmin commented Aug 3, 2015

👍 , @wvengen can you do a rebase?

@recurser

This comment has been minimized.

Show comment
Hide comment

recurser commented Aug 3, 2015

👍

@seuros seuros removed this from the 5.0.0 milestone Aug 3, 2015

@seuros

This comment has been minimized.

Show comment
Hide comment
@seuros

seuros Aug 3, 2015

Member

Just looked at this more closely, the majority of the adapters don't support this feature 😞 .

Member

seuros commented Aug 3, 2015

Just looked at this more closely, the majority of the adapters don't support this feature 😞 .

@wvengen

This comment has been minimized.

Show comment
Hide comment
@wvengen

wvengen Aug 3, 2015

Contributor

@avokhmin yes, I'll have a look!

Contributor

wvengen commented Aug 3, 2015

@avokhmin yes, I'll have a look!

@wvengen

This comment has been minimized.

Show comment
Hide comment
@wvengen

wvengen Aug 17, 2015

Contributor

Rebased on master.
The only small change is that now priority nil is passed to the backend too when unset (which resulted in slightly simpler code). Both backends handle this as equal to not passing the priority argument (I've updated the PR description to reflect this).

Contributor

wvengen commented Aug 17, 2015

Rebased on master.
The only small change is that now priority nil is passed to the backend too when unset (which resulted in slightly simpler code). Both backends handle this as equal to not passing the priority argument (I've updated the PR description to reflect this).

@abhchand

This comment has been minimized.

Show comment
Hide comment
@abhchand

abhchand Sep 17, 2015

👍 Great addition

Looks like this needs another rebase. Any word on progress in getting this merged?

👍 Great addition

Looks like this needs another rebase. Any word on progress in getting this merged?

@wvengen

This comment has been minimized.

Show comment
Hide comment
@wvengen

wvengen Sep 18, 2015

Contributor

Rebased on master.

Contributor

wvengen commented Sep 18, 2015

Rebased on master.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 18, 2015

Member

I'm 👎 on this given that the majority of the adapters don't support priorities (perhaps there's a reason for that 😉).

Member

kaspth commented Sep 18, 2015

I'm 👎 on this given that the majority of the adapters don't support priorities (perhaps there's a reason for that 😉).

@wvengen

This comment has been minimized.

Show comment
Hide comment
@wvengen

wvengen Sep 18, 2015

Contributor

@kaspth I'd say that there are different angles to handling job priority. You can use multiple queues with either multiple workers (most implementations), or workers that handle queues in a specific order (like sidekiq), or assign a priority to jobs (like que). In advanced cases you might even want to use a combination of those.

Contributor

wvengen commented Sep 18, 2015

@kaspth I'd say that there are different angles to handling job priority. You can use multiple queues with either multiple workers (most implementations), or workers that handle queues in a specific order (like sidekiq), or assign a priority to jobs (like que). In advanced cases you might even want to use a combination of those.

@abhchand

This comment has been minimized.

Show comment
Hide comment

Thanks @wvengen!

@deneuxa

This comment has been minimized.

Show comment
Hide comment
@deneuxa

deneuxa Sep 23, 2015

+1 for this feature please :)

deneuxa commented Sep 23, 2015

+1 for this feature please :)

rafaelfranca added a commit that referenced this pull request Sep 26, 2015

@rafaelfranca rafaelfranca merged commit 0f89e15 into rails:master Sep 26, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@wvengen

wvengen Sep 29, 2015

Contributor

Thanks for merging!

Contributor

wvengen commented Sep 29, 2015

Thanks for merging!

@hale

This comment has been minimized.

Show comment
Hide comment
@hale

hale Oct 23, 2015

Thanks everyone. What's the milestone for this change?

hale commented Oct 23, 2015

Thanks everyone. What's the milestone for this change?

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Oct 24, 2015

Member

Rails 5 😄

Member

kaspth commented Oct 24, 2015

Rails 5 😄

@hale

This comment has been minimized.

Show comment
Hide comment

hale commented Oct 24, 2015

👍

@jfine

This comment has been minimized.

Show comment
Hide comment
@jfine

jfine Dec 17, 2015

Contributor

Awesome. Thanks @wvengen!

Contributor

jfine commented Dec 17, 2015

Awesome. Thanks @wvengen!

@adeeb1

This comment has been minimized.

Show comment
Hide comment
@adeeb1

adeeb1 Nov 4, 2016

Sorry to comment on an old PR, but is this supported in or backported to Rails 4.2.7? I am using Delayed Job, and setting the priority in the following manner doesn't set it: MyJob.set(priority: 2).perform_later. The priority is still set to 0.

adeeb1 commented Nov 4, 2016

Sorry to comment on an old PR, but is this supported in or backported to Rails 4.2.7? I am using Delayed Job, and setting the priority in the following manner doesn't set it: MyJob.set(priority: 2).perform_later. The priority is still set to 0.

@deneuxa

This comment has been minimized.

Show comment
Hide comment
@deneuxa

deneuxa Feb 7, 2017

@adeeb1 same problem here, did you solve yours ?
I'm using rails 4.2.3

deneuxa commented Feb 7, 2017

@adeeb1 same problem here, did you solve yours ?
I'm using rails 4.2.3

@wvengen

This comment has been minimized.

Show comment
Hide comment
@wvengen

wvengen Feb 7, 2017

Contributor

@adeeb1 @deneuxa Did you look at the ActiveJob CHANGELOG for 4.2? It doesn't list priorities. If you want them, I think the way to go would be to move to Rails 5.

Contributor

wvengen commented Feb 7, 2017

@adeeb1 @deneuxa Did you look at the ActiveJob CHANGELOG for 4.2? It doesn't list priorities. If you want them, I think the way to go would be to move to Rails 5.

@deneuxa

This comment has been minimized.

Show comment
Hide comment
@deneuxa

deneuxa Feb 7, 2017

thanks @wvengen we will try !

deneuxa commented Feb 7, 2017

thanks @wvengen we will try !

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