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

A single Scheduler for all queues? #14

Open
nvie opened this issue Feb 6, 2013 · 4 comments
Open

A single Scheduler for all queues? #14

nvie opened this issue Feb 6, 2013 · 4 comments

Comments

@nvie
Copy link
Contributor

nvie commented Feb 6, 2013

I'm currently using rq-scheduler in one of my projects, and I was a bit confused when I learned that a scheduler has a queue name argument.

I'm not 100% sure why this feels unnatural to me. Could we make it so that the following is always the case?

scheduler = Scheduler(connection=...)
scheduler.enqueue_at('default', datetime.now(), ...)
scheduler.enqueue_at('high', datetime.now(), ...)
scheduler.schedule('low', ...)

Instead of having three different instances of schedulers for this? I do realize putting the callables onto the queues directly would require three different Queue instances, so I understand your choice for a Scheduler instance belonging to a single Queue.

Nevertheless, I think "a scheduler" is useful to have as a "central component" that you can ask to schedule stuff for you to any queue. Thoughts?

@selwin
Copy link
Contributor

selwin commented Feb 6, 2013

The main reason is that I tried to keep this consistent with Queue's API. You could certainly argue the same thing about Queue being a central component that routes jobs to workers:

q = Queue(connection=conn)
q.enqueue('high', foo)
q.enqueue('default', foo)

Whichever way we decide to do it, I don't mind too much because I think most people would just do (which is still the most elegant way, I think :):

q = Queue('default', connection=conn)
q.schedule_at(time, foo)

If we were to change this though, it'd be better to do it in RQ instead of this repository to avoid backward compatibility issues.

@nvie
Copy link
Contributor Author

nvie commented Feb 6, 2013

You could certainly argue the same thing about Queue being a central component that routes jobs to workers.

Actually, queues are used in RQ for more than just enqueueing stuff, and they are passed around like objects that have a name, which makes sense to bind them to a specific queue. For schedulers, this typically is not the use case, I think.

That said, I think adding the .schedule_at() method to Queues is the most elegant API anyway. In fact, when we have that, I think the queue name argument to Scheduler() can be removed anyway, as can its .enqueue_at() method. Cleaner APIs, FTW!

If we were to change this though, it'd be better to do it in RQ instead of this repository to avoid backward compatibility issues.

Agreed fully.

@ghost
Copy link

ghost commented Nov 4, 2015

+1 for .schedule_at()

@marcinn
Copy link
Contributor

marcinn commented Jan 10, 2019

Hi.

I don't understand how queue handling works currently (0.9, comparing to 0.7). The Scheduler class takes two mutually exclusive args: queue_name and queue. The latter takes higher precedence. Next thing is that Scheduler._create_job() takes a queue_name arg, but it is now silently replaced by self._queue.name if _queue is set.

Another thing is that enqueue_job takes a queue name from job.origin, so no Scheduler.queue_name nor Scheduler._queue is used.

And the last thing - Scheduler.get_jobs() returns jobs registered for Scheduler.scheduled_jobs_key, which is same for all instances regardless queue_name / _queue instance.

It looks somehow inconsistent for me.

My use case was to schedule tasks for run in multiple queues, not only for default. I thought that creating multiple schedulers with different queue names passed as arg is the solution. But it isn't. I've finnally solved this by rolling back all changes (leaving just one Scheduler) and passed queue_name to cron and interval methods explicitely.

As far I understand the code, the self.queue_name is used only as a name of default queue inside _create_job(). I think this can be set as default argument value, not scheduler's property, because:

  • there is no necessity to instantiate multiple Scheduler instances
  • Scheduler.queue_name and Scheduler._queue are introducing inconsistency.

I think that removing queue_name and queue args from Scheduler.__init__ is a way to go. I understand that it would break compatibility.

Just was my 5 cents. Thanks for the great library, anyway!

Kind Regards,
Marcin

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

3 participants