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

[5.6] blocking pop from redis queues #22040

Closed
wants to merge 1 commit into from

Conversation

halaei
Copy link
Contributor

@halaei halaei commented Nov 10, 2017

Currently we pop and reserve jobs off the queue using redis pop and zadd operations inside a LUA script. This PR adds an alternative algorithm that uses blpop instead of pop. It enables the Redis queue driver to wait for the jobs for a specific number of seconds when the queues are empty and server the jobs as soon as they arrive. As a result we can safely run artisan queue:work with no sleep (--sleep=0) without killing the CPU power and improve the wait time of low traffic queues from sleep/2 seconds on average to zero.

The new blocking pop algorithm has a drawback: blpop cannot be used in Lua scripts, therefore, there is a tiny chance of the jobs being lost if the PHP script crashes between pop and reserve operations. Another consideration is that there is no way to do blocking pop from delayed and reserved queues in redis. Therefore, this PR keeps the current pop algorithm in place and lets the user to choose by setting config('queue.connections.{redis_connection_name}.timeout).

PS: I have used blpop for a while via my package, https://github.com/halaei/bredis, that I would like to deprecate if this PR is acceptable.

@halaei halaei force-pushed the blocking-pop-from-redis-queue branch from 4f89faa to 8b3f945 Compare November 10, 2017 10:09
@sisve
Copy link
Contributor

sisve commented Nov 10, 2017

There's some weird naming; timeouts in the queue system usually refer to how long time a job can execute. It also seems that this implementation disallows me to block forever. Perhaps it would be easier to just add a "blpop": { "enabled": true, "timeout": 1337 } setting in the config instead of trying to calculate if blpop should be used based on the timeout specified.

@halaei
Copy link
Contributor Author

halaei commented Nov 10, 2017

You are right about the naming, I just coppied the argument name from redis documentation.
I don't think blpop forever is a good idea. For one thing, it will be in conflict with --timeout param of queue:work. Similar systems, like long-polling in sqs and telegram, don't allow for large values, let alone waiting forever.

@halaei
Copy link
Contributor Author

halaei commented Nov 10, 2017

Any suggestion on what should timeout be renamed to?

@taylorotwell
Copy link
Member

taylorotwell commented Nov 27, 2017

blockFor / --block-for?

@taylorotwell
Copy link
Member

Closing pending inactivity. Feel free to re-open with blockFor --block-for changes.

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.

None yet

3 participants