BLPOP slows down processing

[joeshaw]

Using BLPOP when queues are empty is considerably slower than LPOP because the entire timeout interval must expire before the next queue is checked.

Let's say that you are checking queue1, queue2, and queue3, and there is one job in queue3. The others are empty. Right now, the code does something like this:

while True:
    for q in queues:
        job = redis.blpop(q, timeout=5)
        if job: job.perform(...)

Since queue1 is empty, it takes 5 seconds before the BLPOP times out. Ditto for queue2. The job is popped off queue3 immediately and executed, then the loop runs. Total time for the 3 queues to be traversed: 10 seconds plus the time it took the job to be performed. The second time, with queue3 empty it will take 15 seconds. You can generalize this to interval time x number of empty queues.

With LPOP, the code was structured like this:

while True:
    for q in queues:
        job = redis.lpop(q)
        if job: job.perform(...)
    time.sleep(5)

This meant that every time through the list of queues took 5 seconds, plus the time for the worker, regardless of the number of queues.

This could be solved by running a BLPOP across all the queues simultaneously and rotating the order of the queues to prevent affinity. The downside to this is that BLPOP does not return which queue an item was popped off of. We would have to add this to the serialized job representation, which might cause compatibility issues between pyres versions (not to mention resque). [Wrong. It does return a tuple of redis key and item; the code was just a little confused here.] If that's not an option, I think we should revert to the LPOP implementation. (FWIW, upstream resque still uses LPOP)

[joeshaw]

The commits which switched from LPOP to BLPOP are 48afb8a3a8558bc77c0ee6a985decee819aaedf7 and 44dd501387e8bfe3c93f8192f3db91d375f5f8bd

[joeshaw]

Hmm, I notice from 746413954630489a5b59607a73d5eb0f97920f34 that the key is returned from the redis.blpop() function, at least with version 2.2.2 of the redis-py library. So maybe we can require that version and not need to encode the queue name in the job if we decide to keep the BLPOP route.

[joeshaw]

And I just went back and tested with redis-py 1.34.1, and redis.blpop() there returned a list, not a tuple. So the current code appears to be broken with redis versions that old, but the important thing is that it always returns a sequence of the key as the first arg and the value as the second arg.

I'll try to whip up a patch that keeps the BLPOP.

[binarymatt]

i'd love to see that patch; however, I'm leaning more towards switching back to LPOP. When listening to multiple queues, pyres should always check them in order. That way you can easily create priorities for the worker. When I get home tonight I'll try to take a closer look and see.

[joeshaw]

Ah, ok, if we do want affinity for queues listed earlier I'll omit the reordering.

[joeshaw]

(Is the above sufficient to merge it in, or should I create a pull request for it? I couldn't figure out a better way to attach the code to this issue.)

[binarymatt]

pull request would help a lot. Thanks for the patch, it looks like it's right on.

[binarymatt]

closed via pull request