search

Netflix-Skunkworks / Scumblr

Web framework that allows performing periodic syncs of data sources and performing analysis on the identified results
Apache License 2.0
2.64k stars 318 forks source link

Task scheduling #207

Closed warrent888 closed 7 years ago

warrent888 commented 7 years ago

Adds task scheduling functionality and front-end interface using gem sidekiq-scheduler.

ahoernecke commented 7 years ago

Hi @warrent888, thanks for the pull request, this looks like a great contribution! We haven't gotten a chance to fully review this yet and noticed you made a couple additional commits. Just wanted to see if you thought this PR is ready or if you're still working on it before we start reviewing.

Thanks again!

warrent888 commented 7 years ago

Hi @ahoernecke . I was trying to fix the gems so that the Travis build would succeed, but it seems to be a problem across other PRs as well, so I won't be trying to fix it. This PR is ready for review.

ahoernecke commented 7 years ago

Hi @warrent888,

Just wanted to let you know I'm working on merging this in. I'm thinking about switching the scheduling mechanism over to the cron based based configuration setting. (https://github.com/moove-it/sidekiq-scheduler#schedule-configuration)

Do you see any issues with this for your use case? We feel like this would provide a little extra flexibility since with the scheduling.

warrent888 commented 7 years ago

You're right, this would add more flexibility. But, I couldn't figure out an easy way to write directly to sidekiq.yml from the controller. I also intended to make it so you could schedule tasks without any knowledge of CRON/sidekiq (no need to modify the sidekiq.yml file in a text editor to schedule tasks).

ahoernecke commented 7 years ago

From browsing around it looked like you specify the cron configuration in the same set_schedule method (instead of the "every" key). 

Sidekiq.set_schedule("task: #{id}", { 'every' => [time], 'class' => 'TaskRunner', 'args' => [id] })

Haven't gotten a chance to test this yet so maybe that's not a good assumption.

I was concerned about easy of use as well. I'm thinking about having dropdowns similar to what you used for each of the cron attributes (i.e for hour * or 0-23) on the task index, and then in the task edit form allowing editing/specifying a the cron configuration in a string instead, in case someone needs something more granular.

warrent888 commented 7 years ago

I don't know what you mean by (instead of the "every" key). My understanding is that Sidekiq.set_schedule is pushing tasks to the Redis server to run on the interval specified by 'every'. It would be possible to specify a cron string by setting the 'cron' flag instead of the 'every' flag and then writing a helper method 'cron_to_frequency' or something like that to store into the model.

ahoernecke commented 7 years ago

@sbehrens, @sk3tch would you mind taking a quick review of this PR?

ahoernecke commented 7 years ago

Hi @warrent888, I merged this in this morning. I did end up changing over to cron and also made some changes so that when tasks were deleted, enabled, disabled, etc. and when the application restarts the schedule would be updated.

Thanks again for the PR, this is really a great new feature!

warrent888 commented 7 years ago

Awesome! You're welcome. :)