Closed mattjegan closed 1 year ago
This looks good to me, though one area of concern I have is that rq-scheduler only once per minute adds tasks to the queues. Having the seconds granularity on this may mislead people into thinking that the scheduled time of the repeatable task also works with such precision.
Thanks for the review @tom-price, that's a good point about confusion. If a user was to schedule a task every x seconds they would need to run rq-scheduler like so: rqscheduler -i x
to adjust for the default per minute checking of tasks. Would a note in the README be of use?
I'm still torn on this.
I don't think your suggestion of how to update the readme fits. The get_scheduler
function defined in Django-RQ shown below allows for the interval to be passed in when called by Django-RQ-Scheduler, but so currently there's no way to pass in that interval value. It's not like it's a parameter you can add to RQ_QUEUES
.
def get_scheduler(name='default', interval=60):
"""
Returns an RQ Scheduler instance using parameters defined in
``RQ_QUEUES``
"""
return Scheduler(name, interval=interval,
connection=get_connection(name))
I also don't think that failing silently is an option if the interval set on the job is lower than the scheduler's interval, or maybe isn't even a multiple of the interval frequency (unsure about that part).
In RepeatableJob
functions like these below could be added to throw an error if a save is attempted that violates these conditions; however, the example below relies on a protected member of RQ_Scheduler's Scheduler
class and self.scheduler
would need to be modified to pass in an interval if one other than the default was set, which means you could pull from there if wanting one below 60. I'm going to keep thinking about this.
def clean(self):
super(RepeatableJob, self).clean()
self.clean_interval()
def clean_interval(self):
if self.scheduler()._interval > self.interval_seconds():
raise ValidationError({
'interval_unit': ValidationError(
_("job interval is set lower than queue's interval"), code='invalid')
})
I think the code looks good. Would like to see some documentation with a disclaimer.
@g3rd Thanks for checking this out, do you think the rq-scheduler interval issue needs to be fixed by this PR or just a disclaimer before this is merged?
@g3rd The rq-scheduler interval issue has been resolved on this PR by @tom-price
If everything looks good could you please merge.
@tom-price I have merged in your PR that resolves the duplicate migration. At @divipayhq we have been running this branch in production for nearly 12 months and have not seen any issues with the interval being in the realm of a few seconds. I'd love to see this released as the next version, please let me know if anything is needed @g3rd
Fixes #28
'seconds'
option toRepeatableJob.UNITS