chrisboulton / php-resque

PHP port of resque (Workers and Queueing)
MIT License
3.43k stars 759 forks source link

ability to dequeue jobs of specific queue #211

Closed wedy closed 10 years ago

wedy commented 10 years ago

adds 'dequeue' to remove jobs from a queue @chrisboulton thanks

danhunsaker commented 10 years ago

I'd probably name this method something else, like flush, or even flush_queue, so it's a bit clearer what it does. To me, dequeue is something like a cancel, removing only one job from a queue, rather than all the jobs at once.

I once implemented logic like this - external to Resque itself - which not only allowed me to clear an entire queue, but also let me place the queue on hold by renaming it, so I could then return the jobs to the queue later. One of the neat side effects was the ability to undo a queue flush, which is handy when humans get involved. Might be nice to have that capability here, too...

wedy commented 10 years ago

thanks @danhunsaker, i have updated this PR, hope this is clearer than before, im copying the ruby resque/resque how they handle the 'dequeue', this method can be used to remove a job from a queue, and also can be used to remove all jobs from the queue. if we would like to remove a specific item/class, safely moving each item to a temporary queue before processing it, If the item matches, we ignore it otherwise puts it in a requeue_queue, which at the end it will be copied back into the original queue

chrisboulton commented 10 years ago

I think I'm pretty happy with this to match up to what the Ruby implementation does, in terms of naming and functionality (remove single jobs with a given name, drop an entire queue)

@danhunsaker Pausing a queue is actually something @wedy is working on at the moment for something we're building. We've actually gone down the rename queue approach which you mentioned above (cheers - that's a really good idea), and I think our intent is to build the functionality as a plugin for others interested too.

chrisboulton commented 10 years ago

Forgot the all important: :thumbsup:

danhunsaker commented 10 years ago

That's a bit clearer, yeah. Fair point on the naming, especially if it does handle both (as it does). It seems odd, to me, that the item dequeue operates on the job class, rather than the job ID. I have no doubt that Ruby Redis does things this way, but it seems useful to have the ability to dequeue specific jobs, rather than all jobs of a given type. I agree there's utility to having class-level dequeue, though, and don't want to remove that...

Perhaps an approach where we check the $items array keys, and if they're not numeric, the key is the class, and the value the ID? It might also be useful to allow another approach where if the key is non-numeric, and the value is an array, we treat the key as the class and the value as the job args. That should nicely cover all the levels of specificity we might encounter - specific job, specific job type with specific args, specific job type, and entire queue. Not aware of what Ruby's version allows here, but these could be very useful extensions on both implementations...

danhunsaker commented 10 years ago

I wonder if we should do anything with the job statuses for removed jobs...? Remove the status? Set it to one of the completed statuses so it auto-expires? Add a DEQUEUED status that counts as completed but is more accurate than FINISHED or FAILED?

wedy commented 10 years ago

thanks guys the approach where we check the $items array keys, and if they're not numeric, the key is the class, and the value is the ID, and another approach where if the key is non-numeric, and the value is an array, we treat the key as the class and the value as the job args.

yeah, that's awesome idea, i like it - i will alter this PR this weekend ( if not get merged yet or ill create another PR for this)

not sure what should we do with the job statuses here, any thoughts?

wedy commented 10 years ago

i have altered this PR , make it a bit sexier as we discussed above, unfortunately the ID is not numeric, it's a string hash so in this 2nd commit, it will check Class name or ID Job first , and if 'args' is provided, it will check against args as well. @danhunsaker @chrisboulton what you guys think?

danhunsaker commented 10 years ago

That works, though it isn't quite what I was trying to describe.

I was trying to keep the number of arguments the same as in the Ruby version, so we don't break compatibility if there are any changes "upstream". To do that, we'd need a way to tell what the class argument contains, whether it's a class name, a job ID, or the class and args. I envisioned something along these lines for each (using JSON for simplicity and clarity):

["class"]

{"class": "job ID"}

{"class": ["arg1", "arg2"]}

That setup would make each check pretty straightforward:

foreach ($class as $key => $val) {
    if (is_numeric($key)) {
        // val is class name
    }
    elseif (is_array($val)) {
        // val is arg list
    }
    else {
        // val is job ID
    }
}

Of course, just about any approach will work about as well. I have no idea whether Ruby could do this that way, but this would give us the flexibility to adjust to Runy's approach (assuming it would be different) without breaking apps already using our code at that point. That's my take, anyway.

Still, looks good from here!

wedy commented 10 years ago

i agree, actually i like that better thanks @danhunsaker , more simplicity please find my latest update, now accepting ('queue', json) btw, if you reckon this getting too much, im more than happy to move this to a plugin currently im working on php-resque-pause (https://github.com/wedy/php-resque-pause), so shouldnt be hard for me to re-do it

danhunsaker commented 10 years ago

Here works. Might skip the JSON encode/decode bit, since it's all internal. I just used JSON for my example for clarity. 😊

Otherwise, merge it!

wedy commented 10 years ago

@danhunsaker , ahh cool my bad - updated it , thanks alot for your help

danhunsaker commented 10 years ago

:+1: