-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[12.x] Add ability to ignore queuePaused \ queueShouldRestart cache checks #57975
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
[12.x] Add ability to ignore queuePaused \ queueShouldRestart cache checks #57975
Conversation
|
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
c67ff04 to
ec28c3d
Compare
|
We had a similar PR (#57957) a couple of days ago that got closed. In case you want to avoid these checks, you can try the code I added within a comment there: Changing the if (str_starts_with($key, 'illuminate:queue:')) { |
|
Would it make sense to throw an exception in the queue pausing commands if this static property is set to false? |
|
@cosmastech Yeah, I think that makes sense 🙂 Didn't want to do too much just incase there's no interest in this, But I'll have a poke, thank you 🙏 Edit: I've poked it, but won't add them for now to prevent any scope creep, but happy for either of us / anyone to add them if this is merged / if Taylor likes the idea. |
|
@rodrigopedra Nice one- yeah not sure. I might still try this PR just out of interest as extending it feels awkward I guess. Thank you 🙏 |
|
@jackbayliss any chance we can also get a doc update on how to use this? We also have heavy queues that do not pause / restart, would like to fix this so we aren't making as many cache hits. |
|
@rforced I'll have a poke later tonight unless someone beats me to it! |
|
@jackbayliss I ended up adding |
|
@rforced As long as you're on the latest Laravel version it should work - see the worker class AppServiceProvider should be fine. What's the error you get? I don't use Horizon so would be useful to know :) Alternatively you can call |
|
I think we need to keep restartable enabled, we still need php artisan horizon:terminate to restart our workers (please correct me if I'm understanding this wrong) Here is the warning from Laravel Horizon
|
|
I tested it on a project that uses Horizon and can't reproduce the error. I just added: Worker::$pausable = false;To my project's Some questions:
|
|
I'm not sure what it was @rodrigopedra @jackbayliss, maybe I didn't restart a service but I'm no longer seeing that warning. |
In one of my environments, I have 7+ queue workers, each for specific use cases.
Each queue worker hits the cache to check if it should restart and if it needs to pause for each job.
Using the database cache store means this can quickly become quite heavy - as each worker hits the database. It also appears in Nightwatch a lot, which is what led me to this PR. (ie 7 workers, 2 cache keys - each job - within 24 hours- a lot of cache hits!)
I'm not going to be using these commands (ie restart or pause) - so it would be nice to add the option to disable them & rather than having to extend the cache manager / adjust the queue.worker singleton with some custom solution, I thought this would be nice and at least help the dx.
TLDR - cache keys are hit alot, feels like a lot of overhead if you don't use the restart/pause commands, this adds a way to turn it off.
They default to false so completely opt in, so shouldn't be any breaking changes. Happy to target 13.x if you prefer & open to feedback- thanks!