[5.8] Make the redis queue blocking pop atomic (alternative PR) #27336
Conversation
Use a separate list `queues:{{name}}:notify` to allow us to use the
BLPOP command without risking lost jobs.
…no job and blocking mod enabled
halaei
force-pushed
the
atomic-blocking-pop-2
branch
from
15d19e7 to
67883ff
Compare
|
It is a bit hard to add a test to prevent a bug like the one in 15d19e7 |
halaei
force-pushed
the
atomic-blocking-pop-2
branch
from
e524197 to
0acaf33
Compare
|
So, which PR do you two think is better? 😆 |
| public function testBlockingPop($driver) | ||
| { | ||
| $this->tearDownRedis(); | ||
| if ($pid = pcntl_fork() > 0) { |
There was a problem hiding this comment.
I don't think the pcntl extension is required. You might need to check if it's installed and skip the test if it's not.
|
This PR will perform better while solving most of the issues discussed in #27227. I will close mine. |
| * @param string $from | ||
| * @param string $to | ||
| * @param string $from | ||
| * @param string $to |
There was a problem hiding this comment.
Just a PHPDoc nit: see https://laravel.com/docs/5.7/contributions#coding-style, should have 2 spaces in between.
There's a few spots as well.
|
Would need full upgrade guide / breaking change summary like other PR. |
|
@taylorotwell I can't think of a change with a high chance of impact. But I sent a PR to docs: laravel/docs#4965 |
This is an alternative to #27227. The main difference is in the way these PRs handle some edge cases especially when mixing blocking & non-blocking workers/pushers.
In #27227 non-blocking operations do not interact with notify queue and a script is run to resize the notify queue on every blocking pop.
In this PR non-blocking pushers/workers push/pop from notify queue as well.