Skip to content

Count scheduled vs ready jobs#255

Merged
ukd1 merged 3 commits intomasterfrom
issue_246
Jul 18, 2019
Merged

Count scheduled vs ready jobs#255
ukd1 merged 3 commits intomasterfrom
issue_246

Conversation

@smathieu
Copy link
Contributor

Related to #246

I'm opening this so we can discuss what the interface should look like.

For counting items in a queue, I propose:

q.count(:all)
q.count(:ready)
q.count(:scheduled)

However, I'm a bit stumped when it comes to the QC module itself. Currently, QC.count, would count all the items in the default queue. This is however a bit unexpected. If I'm using multiple queues, I'd expect QC.count to return the count of all the queues. However, changing that behavious would make the count method inconsisten with the other methods in the QC modules that simply delegates to the default queue.

It also does not provide a way to count items in only certain queues. I'd like to be able to do something like QC.count('low', 'medium', 'high') and get only the count from these 3 queues.

Thoughts?

@jipiboily
Copy link
Contributor

I think this all makes a lot of sense!

Related: #246.

@jipiboily
Copy link
Contributor

By that I mean,

q.count(:all)
q.count(:ready)
q.count(:scheduled)

and

QC.count('my_queue') # or multiple queue

make a lot of sense.

@smathieu
Copy link
Contributor Author

What should QC.count('my_queue') return? All the jobs? The ready jobs?

@jipiboily
Copy link
Contributor

@smathieu sigh, good point. Sleep deprived right now :/

I would say all the ready jobs.

We could have:

QC.count_ready('queue') # all ready jobs
QC.count('queue') # alias of count_ready
QC.count_scheduled('queue') # all scheduled jobs

All defaulting to the same 'default' queue? If we were supporting only Ruby 2.0+, I would say that we could use a keyword argument instead, but that's not the case.

@jipiboily
Copy link
Contributor

To expand on my thoughts, if we return the ready jobs, it's the previous behaviour before we had scheduling built-in, that's my reasoning for this behaviour.

@smathieu
Copy link
Contributor Author

What about

# All queues
QC.count
QC.count(:all)
QC.count(:scheduled)
QC.count(:ready)

# Specific queues

QC.count(:all, queues: %w(q1))
QC.count(:all, queues: %w(q1 q2)

# Variable arguments
QC.count(queues: %w(q1)))

Again, my main gripe is that QC.count does not operate of the default queue like the other methods. I can't think of a way of reconciling this behaviour with current system.

Another alternative would be to keep QC.count as is and introduce a new method for QC.count_all that would have the behaviour listed above. But now it's confusing why there's 2 method named count_something.

@jipiboily
Copy link
Contributor

We could actually not change the behaviour of QC.count, leave it as-is and deprecate it but add new methods. It would probably make more sense to not use class methods but having to use something like:

queue = QC::Queue.new(:q1)
queue.count(:ready)
queue.count(:all)
queue.count(:scheduled)
queue.count # this would raise an error, making it more intention revealing by requiring the parameter

If there is a good reason to keep this as class methods, which I can't think of, I would be in favor of introducing new, more intention revealing behaviour, and/or having new methods. If we stick with the same name, we could deprecate the call without a parameter and default it to :all for now and require it in the version after the next?

Many possibilities. I am personally in favor of a lot of code changes that would make it easier to work it, to make it evolve and removing a lot of class methods and possibly non-thread-safe code.

@senny: thoughts on that?

@senny
Copy link
Contributor

senny commented Feb 23, 2015

I'm 👍 to deprecate the class method. I find working with an explicit queue to be much more intention revealing. :ready, :all, :scheduled seems like a good start to improve the Queue#count method. Also falling back to :all for the no argument case and printing a deprecation warning seems like a good idea.

@ukd1 ukd1 merged commit 6049ba0 into master Jul 18, 2019
@ukd1 ukd1 deleted the issue_246 branch July 18, 2019 22:42
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

Successfully merging this pull request may close these issues.

4 participants