Skip to content
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

removing many dynamic schedules at once is not performant #677

Open
ledhed2222 opened this issue Jun 19, 2019 · 7 comments
Open

removing many dynamic schedules at once is not performant #677

ledhed2222 opened this issue Jun 19, 2019 · 7 comments

Comments

@ledhed2222
Copy link

ledhed2222 commented Jun 19, 2019

I have a situation where ~80K dynamic jobs were created before we realized that this approach was not going to work (we're using a static schedule now that is much saner). To clean up all of the persisted schedules using the public interface is straightforward:

(Resque.schedule.keys - YAML.load_file("path", "to", "schedule", "file").keys).each do |to_remove|
  Resque.remove_schedule(to_remove)
end

However, this will not only make one hdel call to redis for each key, but will also re-load the entire schedule each time!

I can imagine a simple change to the remove_schedule method that would allow a variable number of schedules to be removed at once:

def remove_schedule(*schedules)
  schedules.each { |schedule| non_persistent_schedules.delete(schedule) }
  redis.hdel(:persistent_schedules, *schedules)
  redis.sadd(:schedules_changed, schedules.first)
  reload_schedule!
end

What do you guys think about that? Happy to make a PR if that makes sense.

@ledhed2222
Copy link
Author

actually - it appears that we'd only have to update the schedules_changed set with one entry here. updated my proposal above.

@ledhed2222
Copy link
Author

Perhaps a different suggestion. It seems like the root issue here is that there is no story to migrate from dynamic schedules back to a static schedule, if you make the same mistake we did. Instead of the above idea where you can remove many schedules at once, what about purging the entire dynamic schedule:

def purge_dynamic_schedule!
  if !dynamic
    redis.del(:persistent_schedules)
    redis.del(:schedules_changed)
    reload_schedule!
  end
end

this way it's "opt-in": users may wish to drain their dynamic schedules naturally after moving off. but they could optionally just kill the entire

@jeremy
Copy link
Contributor

jeremy commented Jul 9, 2019

@ledhed2222 Bulk removal is welcome! Purging as well. Good call.

@ledhed2222
Copy link
Author

howdy. i'm working on this PR but I'm having trouble passing the test suite. could you provide some insight?

Error: test_concurrent_shutdowns_and_startups_do_not_corrupt_the_schedule(Multi_Process): RuntimeError: forked process failed with pid 43772 SIGABRT (signal 6)
/Users/gregweisbrod/Documents/Dev/resque-scheduler/test/multi_process_test.rb:111:in `block in get_results_from_children'
/Users/gregweisbrod/Documents/Dev/resque-scheduler/test/multi_process_test.rb:108:in `each'
/Users/gregweisbrod/Documents/Dev/resque-scheduler/test/multi_process_test.rb:108:in `get_results_from_children'
/Users/gregweisbrod/Documents/Dev/resque-scheduler/test/multi_process_test.rb:76:in `block (2 levels) in <top (required)>'

there isn't any documentation on your test suite, so even just a few pointers will probably help me here :)

thank you! would love to help get these features in

@ledhed2222
Copy link
Author

Bump 😊

@ledhed2222
Copy link
Author

#686

@ledhed2222
Copy link
Author

bump ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants