Add a retry feature to the backup and clean commands
#1684
Conversation
…ption is encountered
…eption is encountered
…ackup and cleanup commands
src/Commands/BackupCommand.php
Outdated
| @@ -47,6 +51,12 @@ public function handle() | |||
| $backupJob->setFilename($this->option('filename')); | |||
| } | |||
|
|
|||
| if ($this->option('tries')) { | |||
| $this->tries = (int)$this->option('tries'); | |||
| } elseif (!empty(config('backup.backup.tries'))) { | |||
There was a problem hiding this comment.
I really dislike else statements. Could you refactor this to a dedicated method with early returns?
src/Commands/BackupCommand.php
Outdated
| @@ -59,6 +69,15 @@ public function handle() | |||
|
|
|||
| consoleOutput()->comment('Backup completed!'); | |||
| } catch (Exception $exception) { | |||
| if ($this->tries > 1 && $this->currentTry < $this->tries) { | |||
There was a problem hiding this comment.
Conditionals like this are not that readable. Could you refactor this to a dedicated method with a good name, that returns a boolean and uses early returns.
src/Commands/CleanupCommand.php
Outdated
| @@ -42,6 +52,15 @@ public function handle() | |||
|
|
|||
| consoleOutput()->comment('Cleanup completed!'); | |||
| } catch (Exception $exception) { | |||
| if ($this->tries > 1 && $this->currentTry < $this->tries) { | |||
There was a problem hiding this comment.
Refactor to a the conditional to a dedicated method.
src/Commands/BackupCommand.php
Outdated
| @@ -59,6 +69,15 @@ public function handle() | |||
|
|
|||
| consoleOutput()->comment('Backup completed!'); | |||
| } catch (Exception $exception) { | |||
| if ($this->tries > 1 && $this->currentTry < $this->tries) { | |||
| if (!empty(config('backup.backup.retry_delay'))) { | |||
| sleep((int)config('backup.backup.retry_delay')); | |||
There was a problem hiding this comment.
Instead of using sleep use the new sleep helper that is easier to test: https://www.amitmerchant.com/the-new-sleep-helper-in-laravel-10/
…and the new Sleep helper (Laravel 10)
|
Hi, I just made some changes per your comments, let me know if you want anything else updated :-) |
|
The tests are failing for this one, could you take a look |
|
I believe this is due to the fact that the |
|
Feel free to update the minimum requirements, I don't mind to drop support for older versions of Laravel |
…he `Sleep` helper to be available
|
Hi, Just bumped the requirements to >= 10.10.0, let me know if this is good now :-) |
|
Hey @freekmurze, is there anything else I need to add or change to this PR? |
|
Thanks! |
|
Thank you :) |
Hello,
Due to random errors with an S3-compatible backend, I had to create a little retry mechanism for both of these operations to avoid getting spammed of failed backups/cleanups notifications when a simple retry a few seconds later would do the trick.
I believe this is what was requested in #752 as well.
Let me know if anything is missing or incomplete :-)