Add ability to remove individual scheduled jobs [#248] #257

Closed
wants to merge 1 commit into
from

Conversation

7 participants
@jonhinson
Contributor

jonhinson commented Jun 19, 2012

No description provided.

@fred

This comment has been minimized.

Show comment
Hide comment
@fred

fred Jun 27, 2012

That could be a very interesting feature..

fred commented Jun 27, 2012

That could be a very interesting feature..

@linjunpop

This comment has been minimized.

Show comment
Hide comment
@linjunpop

linjunpop Jun 29, 2012

It could be very useful, when this PR will be merged?

It could be very useful, when this PR will be merged?

- pushed = (conn.zadd('schedule', item['at'].to_s, payload) == 1)
+ _, pushed = conn.multi do
+ conn.zadd('schedule', item['at'].to_s, item['at'].to_s)
+ conn.rpush("schedule:#{item['at']}", payload)

This comment has been minimized.

@mperham

mperham Jul 7, 2012

Owner

I don't understand why the rpush is necessary.

@mperham

mperham Jul 7, 2012

Owner

I don't understand why the rpush is necessary.

This comment has been minimized.

@jonhinson

jonhinson Jul 9, 2012

Contributor

When we want to remove the job, we would be unable to remove it based only on the score as there may be multiple jobs scheduled for the same time. Thus, we store the payload in the list with key "schedule:timestamp" and can remove it like so: removed = Sidekiq.redis { |conn| conn.lrem("schedule:#{timestamp}", 0, payload) }

@jonhinson

jonhinson Jul 9, 2012

Contributor

When we want to remove the job, we would be unable to remove it based only on the score as there may be multiple jobs scheduled for the same time. Thus, we store the payload in the list with key "schedule:timestamp" and can remove it like so: removed = Sidekiq.redis { |conn| conn.lrem("schedule:#{timestamp}", 0, payload) }

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Jul 7, 2012

Owner

I would prefer that scheduled jobs check at runtime to see if they should exit early or not.

Consider the case where you schedule an email to a user 5 days from now. 2 days from now, they go into their settings and select "don't send me email, ever". Should checking that box cause the application to go through the Sidekiq queue and remove any scheduled email jobs? I think the better design is to have the email job check if it has the correct permissions and settings when it executes.

Owner

mperham commented Jul 7, 2012

I would prefer that scheduled jobs check at runtime to see if they should exit early or not.

Consider the case where you schedule an email to a user 5 days from now. 2 days from now, they go into their settings and select "don't send me email, ever". Should checking that box cause the application to go through the Sidekiq queue and remove any scheduled email jobs? I think the better design is to have the email job check if it has the correct permissions and settings when it executes.

@fred

This comment has been minimized.

Show comment
Hide comment
@fred

fred Jul 7, 2012

also agree with that, I've changed my code to do the extra checking on the job itself.
removing from queue would be good, but not a required feature, you can always add that logic to your job class.

fred commented Jul 7, 2012

also agree with that, I've changed my code to do the extra checking on the job itself.
removing from queue would be good, but not a required feature, you can always add that logic to your job class.

@jonhinson

This comment has been minimized.

Show comment
Hide comment
@jonhinson

jonhinson Jul 9, 2012

Contributor

@mperham Consider an application such as google calendar that has reminders for events. Say you set 3 reminders per event. If the event changes N times, then you'd have N * 3 "phantom" jobs that don't have permission to run. It seems a little more explicit and clear to simply remove the old jobs and create the new ones.

Contributor

jonhinson commented Jul 9, 2012

@mperham Consider an application such as google calendar that has reminders for events. Say you set 3 reminders per event. If the event changes N times, then you'd have N * 3 "phantom" jobs that don't have permission to run. It seems a little more explicit and clear to simply remove the old jobs and create the new ones.

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Jul 10, 2012

Owner

Yeah, given that jobs don't have a unique identity, e.g. an 'id' property, we must resort to hacks to find the job to remove. On top of this, removing a job feels akin to cache invalidation: very easy to get wrong within your application code.

I'm sticking with my original thesis: jobs should check if they are still relevant/useful when run.

Owner

mperham commented Jul 10, 2012

Yeah, given that jobs don't have a unique identity, e.g. an 'id' property, we must resort to hacks to find the job to remove. On top of this, removing a job feels akin to cache invalidation: very easy to get wrong within your application code.

I'm sticking with my original thesis: jobs should check if they are still relevant/useful when run.

@mperham mperham closed this Jul 10, 2012

@rmello

This comment has been minimized.

Show comment
Hide comment
@rmello

rmello Oct 25, 2012

Contributor

Now you seem to be using SecureRandom to generate an unique ID to each item. It should be easy to store the payload on a Hash linked to the jid. Is the issue still relevant?

Contributor

rmello commented Oct 25, 2012

Now you seem to be using SecureRandom to generate an unique ID to each item. It should be easy to store the payload on a Hash linked to the jid. Is the issue still relevant?

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Oct 25, 2012

Owner

There is now an API for enumerating and removing jobs from the retry and named queues. See sidekiq/api

Owner

mperham commented Oct 25, 2012

There is now an API for enumerating and removing jobs from the retry and named queues. See sidekiq/api

@rmello

This comment has been minimized.

Show comment
Hide comment
@rmello

rmello Oct 29, 2012

Contributor

What about the "schedule" queue? I guess it should have a special class (like the retry queue), right?

Contributor

rmello commented Oct 29, 2012

What about the "schedule" queue? I guess it should have a special class (like the retry queue), right?

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Oct 29, 2012

Owner

@rmello Yeah, I realized that oversight last night. I'll fix it.

Owner

mperham commented Oct 29, 2012

@rmello Yeah, I realized that oversight last night. I'll fix it.

@rmello

This comment has been minimized.

Show comment
Hide comment
@rmello

rmello Oct 29, 2012

Contributor

It would be a great plus if one could pass the time range on the enumerator to allow enumerating only on a specific "time frame" scheduled jobs (avoiding the parse of all the scheduled JSON jobs - since afaik it will not be possible to directly access a job through its jid). Dont know if it will be performance relevant - Just a thought :)

Contributor

rmello commented Oct 29, 2012

It would be a great plus if one could pass the time range on the enumerator to allow enumerating only on a specific "time frame" scheduled jobs (avoiding the parse of all the scheduled JSON jobs - since afaik it will not be possible to directly access a job through its jid). Dont know if it will be performance relevant - Just a thought :)

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Oct 31, 2012

Owner

Sidekiq 2.5 includes the new API. I did not add the time range feature but would welcome a PR adding it.

Owner

mperham commented Oct 31, 2012

Sidekiq 2.5 includes the new API. I did not add the time range feature but would welcome a PR adding it.

@jhilden

This comment has been minimized.

Show comment
Hide comment
@jhilden

jhilden Oct 31, 2012

Contributor

Is there any documentation yet on how to use this new feature?

Contributor

jhilden commented Oct 31, 2012

Is there any documentation yet on how to use this new feature?

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Oct 31, 2012

Owner

Not yet. You can read through api.rb, there's some rdoc with basic examples.

Owner

mperham commented Oct 31, 2012

Not yet. You can read through api.rb, there's some rdoc with basic examples.

@tmaier tmaier referenced this pull request in lardawge/carrierwave_backgrounder Mar 20, 2013

Merged

Fail silently when record can't be found in workers. #123

@yanismydj

This comment has been minimized.

Show comment
Hide comment
@yanismydj

yanismydj May 5, 2013

I just wanted to see if there was any progress on this perhaps

I just wanted to see if there was any progress on this perhaps

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham May 5, 2013

Owner

@yanismydj Define this.

Owner

mperham commented May 5, 2013

@yanismydj Define this.

@yanismydj

This comment has been minimized.

Show comment
Hide comment
@yanismydj

yanismydj May 6, 2013

Sorry I should have been more specific. I was curious about the status of documentation on this. I ended up looking through api.rb and figured it out. Thanks @mperham

Sorry I should have been more specific. I was curious about the status of documentation on this. I ended up looking through api.rb and figured it out. Thanks @mperham

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