Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Basic support for blocking list pop #63

Merged
merged 22 commits into from Jun 12, 2013

Conversation

Projects
None yet
6 participants
Contributor

ruudk commented Aug 24, 2012

This PR has basic support for BLPOP. Since PR #58 has a lot more changes in it than what's needed I decided to recreate it.

The tests still use LPOP because that's faster to test. BLPOP differs in a small way (that it returns an array) which is not tested but I don't see any problem in that.

This pull request fails (merged 41bf8c1 into 64cd187).

Contributor

ruudk commented Sep 10, 2012

The PR now passes tests (http://travis-ci.org/#!/chrisboulton/php-resque/builds/2392749) but I don't see it in the comments here. Weird.

jbrower commented Dec 4, 2012

Is there any update on this? I'd love to be using blocking operations for this so that I don't have to wait for the interval.

Merge branch 'master' of git://github.com/chrisboulton/php-resque int…
…o blocking-list-pop

Conflicts:
	lib/Resque.php
	lib/Resque/RedisCluster.php
	lib/Resque/Worker.php
Contributor

ruudk commented Mar 11, 2013

@chrisboulton I've updated the PR with the latest changes of master. Tests seem ok with REDIS_STANDALONE=1 but not with REDIS_STANDALONE=0. Any ideas?

https://travis-ci.org/chrisboulton/php-resque/builds/5423898

Contributor

ruudk commented Mar 12, 2013

I can't get it to work because there is an issue with Credis. Credis errors on using blpop. When I use the native PHP Redis extension it works fine. I'll work this out with the Credis maintainer first.

See: colinmollenhour/credis#8

jbrower commented Mar 12, 2013

@ruudk With that fix being merged into Redis now, does this seem to be working? I'm very excited about using blpop and such. :)

Contributor

ruudk commented Mar 12, 2013

@jbrower Not jet, but I'm working on it. I have some uncommitted stuff that I need to test further. Stay tuned :)

Contributor

ruudk commented Mar 13, 2013

@jbrower @chrisboulton Ok, I've got it working properly.

  • Pull my PR
  • Run composer install
  • Open terminal and cd demo
  • Run VVERBOSE=1 QUEUE=job1 INTERVAL=5 BLOCKING=1 php resque.php
  • In a new terminal window run php queue.php job1 PHP_Job in the demo folder

I've added an extra BLOCKING env variable so that people can choose to use this or not. Default is no blocking list pop.

The only problem I'm seeing right now is that when you use blocking list pop it's not possible to kill the worker. So when you set a timeout of 60 seconds on the blpop command, you'll have to wait 60 seconds before you can kill it.

One way to get rid of this is by letting the blpop command also monitor a special list where it can receive kill commands. Something like this:

BLPOP resque:worker:kill resque:queue:my-queue-1 resque:queue:my-hooks-queue 60

Now when you want to kill the worker, you add kill commands to the resque:worker:kill list.

Contributor

ruudk commented Mar 14, 2013

@jbrower I just talked to chris and he is on vacation for 6 weeks so we'll have to wait for a while :)

Update Job.php
Previous code was encapsulating $args in an additional array, and thus breaking the new job $args.

jbrower commented Apr 17, 2013

Any news on this? My coworker is giving a presentation at the Openwest Conference (openwest.org) on using workers in PHP. It'd be really nice to have this functionality in so that people can more easily see the benefits rather than waiting a few seconds when a new job is created.

Contributor

ruudk commented Apr 18, 2013

I think @chrisboulton is still on vacation for another 2 weeks :(

Contributor

cballou commented May 2, 2013

Ah, it'd be nice to see this merged 👍

@chrisboulton chrisboulton commented on the diff May 3, 2013

lib/Resque/Worker.php
- $this->log($status, self::LOG_VERBOSE);
-
- // Wait until the child process finishes before continuing
- pcntl_wait($status);
- $exitStatus = pcntl_wexitstatus($status);
- if($exitStatus !== 0) {
- $job->fail(new Resque_Job_DirtyExitException(
- 'Job exited with exit code ' . $exitStatus
- ));
- }
- }
-
- $this->child = null;
- $this->doneWorking();
- }
+ while(true) {
@chrisboulton

chrisboulton May 3, 2013

Owner

Has something funky happened with the indentation here?

@ruudk

ruudk May 3, 2013

Contributor

I think PHPStorm automatically changed this. Do you use 2 space intentation?

Contributor

ruudk commented May 22, 2013

@chrisboulton What's the status of this? Why does this take so long to merge.... :(

Owner

chrisboulton commented May 29, 2013

Sorry - I'm so busy these days that I've let a few things fall behind.

If you can do me a favor and rebase off the latest master, I'll merge ASAP. I have to find a bit of time testing, so after this merge I'd definitely call master unstable (I think it pretty much is given the number of changes since the last release)

Contributor

ruudk commented May 29, 2013

Ok rebased. Can you check it out? Please first review it as a separate branch before merging :)

So basicly INTERVAL=0 means blocking list pop mode.

Owner

chrisboulton commented May 29, 2013

Thanks mate.

Have just fetched the branch and am taking a look now.

Quick question - what's the difference between bin/resque and bin/resque.php and the intent for creating resque.php?

Edit: It also looks like there are some changes in bin/resque.php that don't exist in bin/resque:

--- resque  2013-05-29 20:18:38.000000000 +1000
+++ resque.php  2013-05-29 20:18:38.000000000 +1000
@@ -1,6 +1,4 @@
-#!/usr/bin/env php
 <?php
-
 // Find and initialize Composer
 $files = array(
   __DIR__ . '/../../vendor/autoload.php',
@@ -50,6 +48,8 @@
    $logLevel = Resque_Worker::LOG_VERBOSE;
 }

+$BLOCKING = getenv('BLOCKING') !== FALSE;
+
 $APP_INCLUDE = getenv('APP_INCLUDE');
 if($APP_INCLUDE) {
    if(!file_exists($APP_INCLUDE)) {
@@ -71,12 +71,6 @@
    $count = $COUNT;
 }

-$PREFIX = getenv('PREFIX');
-if(!empty($PREFIX)) {
-    fwrite(STDOUT, '*** Prefix set to '.$PREFIX."\n");
-    Resque_Redis::prefix($PREFIX);
-}
-
 if($count > 1) {
    for($i = 0; $i < $count; ++$i) {
        $pid = Resque::fork();
@@ -89,7 +83,7 @@
            $worker = new Resque_Worker($queues);
            $worker->logLevel = $logLevel;
            fwrite(STDOUT, '*** Starting worker '.$worker."\n");
-           $worker->work($interval);
+            $worker->work($interval, $BLOCKING);
            break;
        }
    }
@@ -107,6 +101,5 @@
    }

    fwrite(STDOUT, '*** Starting worker '.$worker."\n");
-   $worker->work($interval);
+    $worker->work($interval, $BLOCKING);
 }
-?>
Contributor

ruudk commented May 29, 2013

Sorry, that must have been a merge conflict/bug. Because my PR was so old I had a bit of trouble rebasing :)

Owner

chrisboulton commented Jun 1, 2013

So I'm happy to fix this, but would prefer it to all be done in the context of the pull request, rather than me updating it after. The only problem is, I can't push back to your branch to update the PR.

Would you be able to sort out bin/resque? Ideally I'd like bin/resque.php's changes folded back into bin/resque.

Everything else looks good, except I am having some issues when using phpredis. Looking into that some more now.

Contributor

ruudk commented Jun 3, 2013

Ok should work now.

To test things out. Checkout my PR and run composer install. Then use:

QUEUE=test VVERBOSE=1 INTERVAL=5 BLOCKING=1 ./bin/resque
Contributor

ruudk commented Jun 7, 2013

@chrisboulton Merge it 👍 👍

Contributor

ruudk commented Jun 12, 2013

ruudk opened this pull request 10 months ago

Good to merge — The Travis CI build passed

:(

Owner

chrisboulton commented Jun 12, 2013

Going to merge, but haven't had the opportunity to fully test this (along with most of master). Sorry for the delay.

chrisboulton added a commit that referenced this pull request Jun 12, 2013

Merge pull request #63 from ruudk/blocking-list-pop
Basic support for blocking list pop

@chrisboulton chrisboulton merged commit 59f617e into chrisboulton:master Jun 12, 2013

1 check passed

default The Travis CI build passed
Details
Contributor

ruudk commented Jun 17, 2013

@chrisboulton Thanks for merging. I've been using this in production for a couple of days now and it works fine. Time for a new tagged release!

@ruudk ruudk referenced this pull request Dec 5, 2013

Closed

Use BLPOP by default? #150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment