Efficent remove_delayed implementation #167

Merged
merged 15 commits into from Sep 6, 2013

Conversation

Projects
None yet
9 participants
Member

bugant commented Jun 28, 2012

Using Redis keys command could be really dangerous: it may ruin performance when it is executed against large databases. See the 'Warning' section at http://redis.io/commands/keys.

In the attached commits, for every job a set keeps track of the delayed keys where it appears. This obviously stores redundant data but makes remove_delayed performance savvy.

bugant and others added some commits Jun 28, 2012

@bugant bugant FIX: avoid using Redis keys command since it scans the whole db keys
Keep track of the timestamps at which a give job occurs in a separate
set. This is redundant but useful if you want to provide a way to remove
scheduled jobs without potentially incurring in seriously performance
issues.
9471b2d
@bugant bugant Add a migration script to be used to migrate to
the new job timestamps tracking on an existant
resque-scheduler redis db.
a4c1c23
@potomak potomak Add scheduled_at method
It returns an array of timestamps the selected job is scheduled at
0760fac
@bugant bugant Merge pull request #1 from potomak/master
Find job schedules time
1ccad5c
@bugant bugant Delete set tracking timestamps on remove_delayed
When removing all occurencies of a given delayed item remove also the
key used to track the timestamps at which the job was scheduled.
d68b6ba
@bugant bugant Merge branch 'master' of github.com:bugant/resque-scheduler a1e7d85
@bugant bugant Remove items from the timestamps set instead of deleting it
Remove items instead of deleting the timestamp set. This prevent to
delete some new jobs added while deleting the old ones.
79e9fe5
@bugant bugant Remove elements from the timestamps set in next_item_for_timestamp an…
…d remove_delayed_job_from_timestamp
0b2ec01
@bugant bugant Add test to check that after reset no timestamps set is left 0050171

+1 – I had no idea that remove_delayed was as slow as it is, and got bit by using it synchronously.

chewi added some commits Aug 22, 2012

Contributor

chewi commented Aug 22, 2012

Please see bugant/resque-scheduler#2 for some additional commits.

bugant added some commits Aug 23, 2012

@bugant bugant Merge pull request #2 from chewi/master
Add enqueue_delayed for enqueing specific delayed jobs immediately
bd26ce5
@bugant bugant Make the replies processing more clear a1731c1
Owner

bugant commented on a1731c1 Aug 23, 2012

@chewi are you ok with this? I think it is more readable this way

Yes, if we're happy that the lrem call will definitely always return an integer. Or even...

(replies.nil? || replies.empty?) ? 0 : replies.each_slice(2).collect(&:first).inject(:+)
Contributor

sreeix commented Sep 20, 2012

+1 on this change, we use it in the user request path and we have a lot of keys in the Resque.redis

ches commented Oct 25, 2012

👍 as well, this is a pretty critical issue IMO.

Contributor

meatballhat commented Sep 4, 2013

@bugant #185 merge + other changes introduced conflicts. Still interested in having your changes merged, or is #185 sufficient?

Contributor

chewi commented Sep 4, 2013

I'm afraid I don't use Resque any more. Hopefully @bugant is still interested.

Member

bugant commented Sep 4, 2013

@meatballhat #185 resolves the issue with the keys command. It is not compatible with #167.

#167 is more intrusive though add some functionalities such as scheduled_at method (bugant/resque-scheduler@0760fac)

Contributor

meatballhat commented Sep 5, 2013

@bugant I'd be happy to take a look at whatever you'd like to submit if you're interested in contributing any part of your changes 😸

Member

bugant commented Sep 5, 2013

@meatballhat sure I'm still interested if you think it could be useful for the project. I'll try to apply my changes to latest master and see what's causing conflicts.

Contributor

meatballhat commented Sep 5, 2013

Thanks @bugant!

Member

bugant commented Sep 6, 2013

@meatballhat I've merged upstream master in mine and fixed the only conflict I got with lib/resuqe_scheduler.rb

Tests are all green here. Hope it helps.

Contributor

meatballhat commented Sep 6, 2013

@bugant I like it 😄, thanks for reworking! Do you have any plans to integrate any part of what you've added to the web view?

@meatballhat meatballhat added a commit that referenced this pull request Sep 6, 2013

@meatballhat meatballhat Merge pull request #167 from bugant/master
Efficent remove_delayed implementation
670a835

@meatballhat meatballhat merged commit 670a835 into resque:master Sep 6, 2013

1 check passed

default The Travis CI build passed
Details
Member

bugant commented Sep 6, 2013

@meatballhat nice! 👍 I didn't think about the web view originally, it could be useful though. I'll try to give it a look

Contributor

meatballhat commented Sep 6, 2013

@bugant Thanks again for contributing!

This merge surprised me, considering #185 was already merged. Its not immediately obvious what functionality was gained/improved by this change - can someone help me out?

Contributor

meatballhat commented Sep 6, 2013

@joshuaflanagan The feature I wanted was a formalized mechanism for querying for when jobs are scheduled, which is why I followed up with @bugant about possibly adding UI integration. The other "feature" I wanted was @bugant's continued involvement. If you still feel that I shouldn't have merged it, I'd love to talk about it.

Member

bugant commented Sep 10, 2013

@meatballhat Do you have any idea on how to add the new feature to the web UI. I mean, do you think it would be ok to have job to be clickable and linked to a page showing the list of timestamp for which the job is currently scheduled?

Contributor

meatballhat commented Sep 10, 2013

@bugant That sounds like a reasonable enough start to me. Give it a shot, take some screenshots, and we'll solicit feedback.

Member

bugant commented Nov 4, 2013

@meatballhat I found some time to work on it on the weekend. I need to finish it up, but I'll send a PR soon with a basic implementation.

Contributor

meatballhat commented Nov 5, 2013

@bugant awesome!

bugant referenced this pull request Nov 5, 2013

Merged

View all schedules for a job #282

@bjones bjones pushed a commit to bjones/resque-scheduler that referenced this pull request May 30, 2014

@meatballhat meatballhat Merge pull request #167 from bugant/master
Efficent remove_delayed implementation
8612bab

If you're still out there, this script is a lifesaver. Would have been nice to see it documented in a more official capacity!

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