[11.x] Adds support to allow specifying number of days to keep failed jobs in DynamoDB #54295
+18
−8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
morloderex
reviewed
Jan 22, 2025
There was a problem hiding this comment.
The property expireDays should be an integer, not a string.
Casting to an integer doesn't seem to be the correct approach here.
I have also defaulting the constructor argument to be the integer 3.
Let's wait for future feedback, as usually changing a constructor argument is considered a breaking change. So it might be better to add this additional configuration value through a getter and setter methods instead.
Comment on lines
+35
to
+40
| /** | ||
| * The number of days a failed job should be retained. | ||
| * | ||
| * @var string | ||
| */ | ||
| protected $expireDays; |
There was a problem hiding this comment.
Suggested change
| /** | |
| * The number of days a failed job should be retained. | |
| * | |
| * @var string | |
| */ | |
| protected $expireDays; | |
| /** | |
| * The number of days a failed job should be retained. | |
| * | |
| * @var int | |
| */ | |
| protected $expireDays; |
| { | ||
| $this->table = $table; | ||
| $this->dynamo = $dynamo; | ||
| $this->applicationName = $applicationName; | ||
| $this->expireDays = (int) $expireDays; |
There was a problem hiding this comment.
Suggested change
| $this->expireDays = (int) $expireDays; | |
| $this->expireDays = $expireDays; |
| @@ -324,7 +324,8 @@ protected function dynamoFailedJobProvider($config) | |||
| return new DynamoDbFailedJobProvider( | |||
| new DynamoDbClient($dynamoConfig), | |||
| $this->app['config']['app.name'], | |||
| $config['table'] | |||
| $config['table'], | |||
| $config['expire_days'] ?? '3' | |||
There was a problem hiding this comment.
Suggested change
| $config['expire_days'] ?? '3' | |
| $config['expire_days'] ?? 3 |
| @@ -47,7 +47,7 @@ public function testCanProperlyLogFailedJob() | |||
| ], | |||
| ]); | |||
|
|
|||
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table'); | |||
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', '3'); | |||
There was a problem hiding this comment.
Suggested change
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', '3'); | |
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', 3); |
| @@ -83,7 +83,7 @@ public function testCanRetrieveAllFailedJobs() | |||
| ], | |||
| ]); | |||
|
|
|||
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table'); | |||
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', '3'); | |||
There was a problem hiding this comment.
Suggested change
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', '3'); | |
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', 3); |
| @@ -124,7 +124,7 @@ public function testASingleJobCanBeFound() | |||
| ], | |||
| ]); | |||
|
|
|||
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table'); | |||
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', '3'); | |||
There was a problem hiding this comment.
Suggested change
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', '3'); | |
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', 3); |
| @@ -152,7 +152,7 @@ public function testNullIsReturnedIfJobNotFound() | |||
| ], | |||
| ])->andReturn([]); | |||
|
|
|||
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table'); | |||
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', '3'); | |||
There was a problem hiding this comment.
Suggested change
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', '3'); | |
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', 3); |
| @@ -171,7 +171,7 @@ public function testJobsCanBeDeleted() | |||
| ], | |||
| ])->andReturn([]); | |||
|
|
|||
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table'); | |||
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', '3'); | |||
There was a problem hiding this comment.
Suggested change
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', '3'); | |
| $provider = new DynamoDbFailedJobProvider($dynamoDbClient, 'application', 'table', 3); |
| * @return void | ||
| */ | ||
| public function __construct(DynamoDbClient $dynamo, $applicationName, $table) | ||
| public function __construct(DynamoDbClient $dynamo, $applicationName, $table, $expireDays) |
There was a problem hiding this comment.
Suggested change
| public function __construct(DynamoDbClient $dynamo, $applicationName, $table, $expireDays) | |
| public function __construct(DynamoDbClient $dynamo, $applicationName, $table, $expireDays = 3) |
| /** | ||
| * Create a new DynamoDb failed job provider. | ||
| * | ||
| * @param \Aws\DynamoDb\DynamoDbClient $dynamo | ||
| * @param string $applicationName | ||
| * @param string $table | ||
| * @param string $expireDays |
There was a problem hiding this comment.
Suggested change
| * @param string $expireDays | |
| * @param int $expireDays |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a new config option for queue.failed (when using DynamoDB):
expire_daysThis allows controlling the expires_at value for jobs that are inserted into DynamoDB, instead of the hardcoded value of 3. (Still defaulted to 3 in the absence of the config option)
This pull request introduces changes to the
DynamoDbFailedJobProviderclass to make the expiration period for failed jobs configurable. The main modifications include adding a new property for expiration days, updating the constructor to accept this new parameter, and adjusting the tests to accommodate this change.Key changes:
Configuration of Expiration Days:
src/Illuminate/Queue/Failed/DynamoDbFailedJobProvider.php: Added a new protected propertyexpireDays, updated the constructor to acceptexpireDaysas a parameter, and modified thelogmethod to use the configurable expiration period. [1] [2]Service Provider Adjustment:
src/Illuminate/Queue/QueueServiceProvider.php: Updated thedynamoFailedJobProvidermethod to pass theexpire_daysconfiguration value, defaulting to '3' if not provided.Test Updates:
tests/Queue/DynamoDbFailedJobProviderTest.php: Updated multiple test methods to include theexpireDaysparameter when creatingDynamoDbFailedJobProviderinstances. [1] [2] [3] [4] [5]