Added onOneServer option to short run commands #8
Conversation
…re illuminate/cache. Added onOneServer method and protected property to PendingShortScheduleCommand. Refactored the run command to run one private method depending on the onOneServer value. OneOneServer still calls the processCommand method that do not have the onOneServer property set. Add a test to ensure the cache item exists when onOneServer is true and run
src/ShortScheduleCommand.php
Outdated
| private function processOnOneServer() | ||
| { | ||
| if (Cache::missing($this->pendingShortScheduleCommand->cacheName())) { | ||
| Cache::add($this->pendingShortScheduleCommand->cacheName(), true, 60); |
There was a problem hiding this comment.
Why 60? Does the Laravel framework code also uses this value?
There was a problem hiding this comment.
The framework was using 3600 but I thought 60 seconds as this whole package is about sub minute tasks. I was thinking it should use
$this->pendingShortScheduleCommand->frequencyInSeconds()
so the expiration lasts only as long as the time between checks. However this should also depend on if you care about overlaps or not.
If it's too short could be an issue for some and same if it's too long. I assume the frameworks assumption no task will run for more than an hour.
There was a problem hiding this comment.
Ok, let's leave it at 60.
Should we also have to clean up this file when starting the loop? Right now, if we were to abort the loop at the wrong time, the file would stay, keeping to lock longer than it needs to be there.
There was a problem hiding this comment.
You mean in the situation the short-schedule:run is killed or dies then when it starts again that the command will be stuck because if the cache key is set? I have not experienced this. I am not 100% sure the running command actually terminates at the same time as as the loop.
So short-schedule:run can stop and I think the process that was running can still run and finish. It just will not be called again.
Worst case the key will expire in 1 minute. So in theory it should start running on one sever after 60 seconds in worst case scenario.
|
The tests are failing for this PR, could you take a look at that? |
|
@freekmurze have added a test and the tests now pass |
|
Thanks! |
This is to fore fill the Issue I previously created #6
Update README.md to include new method.
Update composer.json to require illuminate/cache.
Added onOneServer method and protected property to PendingShortScheduleCommand.
Refactored the run command to run one private method depending on the onOneServer value. OneOneServer still calls the processCommand method that do not have the onOneServer property set.
Add a test to ensure the cache item exists when onOneServer is true and run.
I am not sure if my test is the best way to ensure success. It should have the flag set in Cache if using the option.
I did a lot of debugging and testing in local and positive this works. It does for my needed use case. I did not want to leave logging in the code in order to test against so maybe you have a better way of testing this than I to assure yourself.