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

add seconds option to interval_units with tests #29

Closed
wants to merge 23 commits into from

Conversation

mattjegan
Copy link

Fixes #28

  • Adds the 'seconds' option to RepeatableJob.UNITS
  • Adds test for seconds unit
  • Adds ignore rules for Pipfile so I didn't clutter the repo

@tom-price
Copy link
Collaborator

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.

@mattjegan
Copy link
Author

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?

@tom-price
Copy link
Collaborator

tom-price commented May 26, 2019

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')
            })

Rejects when a job's interval is lower than a queue's interval.
Rejects when a job's interval is not a multiple of a queue's interval. This seemed to be the most restrictive approach from which things can be loosened up.
Added a settings parameter DJANGO_RQ_SCHEDULER_INTERVAL that can be used to change the schedular interval. Default is 60 matching DJANGO_RQ's default.
Renamed migration adding seconds to be more descriptive.
@g3rd
Copy link
Collaborator

g3rd commented Jan 8, 2020

I think the code looks good.
Would like to see some documentation with a disclaimer.

@mattjegan
Copy link
Author

@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?

@mattjegan
Copy link
Author

@g3rd The rq-scheduler interval issue has been resolved on this PR by @tom-price

If everything looks good could you please merge.

Copy link
Collaborator

@tom-price tom-price left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this merge cbd6d13 introduced a 2nd migration 0006_{migration name}.py breaking this PR.

@mattjegan
Copy link
Author

mattjegan commented Jul 14, 2021

@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

@mattjegan mattjegan closed this Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add seconds as a option RepeatableJob.UNITS
4 participants