Preserve queue job items and cleanup after some time #472
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
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe changes reflect a comprehensive update to a PHP application's queue system, enhancing queue management, job dispatching, and scheduling. New features include batch size configuration, concurrent batch limits, queue item retention settings, and an admin interface for queue management. The application's service provider bindings and core aliases have been updated, and new methods and properties have been added across various classes to improve queue job handling, error logging, and event management. PHP 8 features like constructor property promotion and enums are utilized, indicating a modernization of the codebase. Changes
Note: The ellipsis (...) in the file paths indicates that the path has been shortened to group similar changes and save space. TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Comment on lines
+133
to
137
| * @param class-string $job Job class to run. | ||
| * @param array $arguments Arguments for the command. | ||
| * @return Event | ||
| * | ||
| * @throws RuntimeException Thrown on missing command. |
There was a problem hiding this comment.
Similar to the command method, the job method has been updated to include a type declaration for the $job parameter. The comment on line 137 has a typo and should be corrected from "Thrown invalid command class." to "Thrown if an invalid job class is passed." to accurately reflect the exception being thrown.
- * @throws RuntimeException Thrown invalid command class..
+ * @throws RuntimeException Thrown if an invalid job class is passed.Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| * @param class-string $job Job class to run. | |
| * @param array $arguments Arguments for the command. | |
| * @return Event | |
| * | |
| * @throws RuntimeException Thrown on missing command. | |
| * @param class-string $job Job class to run. | |
| * @param array $arguments Arguments for the command. | |
| * @return Event | |
| * | |
| * @throws RuntimeException Thrown if an invalid job class is passed. |
Comment on lines
+1
to
+48
| <?php | ||
| /** | ||
| * Cleanup_Jobs_Commands class file. | ||
| * | ||
| * @package Mantle | ||
| */ | ||
|
|
||
| namespace Mantle\Queue\Console; | ||
|
|
||
| use Mantle\Console\Command; | ||
| use Mantle\Contracts\Container; | ||
| use Mantle\Queue\Events\Job_Processed; | ||
| use Mantle\Queue\Events\Job_Processing; | ||
| use Mantle\Queue\Events\Run_Complete; | ||
| use Mantle\Queue\Events\Run_Start; | ||
| use Mantle\Queue\Providers\WordPress\Post_Status; | ||
| use Mantle\Queue\Providers\WordPress\Queue_Record; | ||
|
|
||
| /** | ||
| * Queue Cleanup Command | ||
| */ | ||
| class Cleanup_Jobs_Command extends Command { | ||
| /** | ||
| * The console command name. | ||
| * | ||
| * @var string | ||
| */ | ||
| protected $name = 'queue:cleanup'; | ||
| /** | ||
| * Command Description. | ||
| * | ||
| * @var string | ||
| */ | ||
| protected $description = 'Cleanup old queue jobs.'; | ||
|
|
||
| /** | ||
| * Command action. | ||
| */ | ||
| public function handle() { | ||
| Queue_Record::query() | ||
| ->whereStatus( [ Post_Status::FAILED->value, Post_Status::COMPLETED->value ] ) | ||
| ->olderThan( now()->subSeconds( (int) $this->container['config']->get( 'queue.delete_after', 60 ) ) ) | ||
| ->take( -1 ) | ||
| ->each_by_id( | ||
| fn ( Queue_Record $record ) => $record->delete( true ), | ||
| 100, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The class Cleanup_Jobs_Command is well-structured and follows good object-oriented design principles. It extends the base Command class and defines properties and a method that are clear in their purpose. The handle method uses a fluent interface to build a query, which is a common and readable way to interact with models in modern PHP frameworks.
However, there are a few points to consider:
- The use of PHP 8's enums (
Post_Status::FAILED->value,Post_Status::COMPLETED->value) is a good practice for code clarity and maintainability. Ensure that the rest of the codebase is compatible with PHP 8 features if they are being used here. - The
olderThanmethod seems to be a custom addition to the query builder. Ensure that this method is properly defined and tested in theQueue_Recordmodel or query builder class. - The
take(-1)method call is unusual. Typically,takeis used to limit the number of records returned. If-1is intended to indicate "all records," this should be clearly documented, as it's not a standard convention. If it's a mistake, it should be corrected. - The lambda function
fn (Queue_Record $record) => $record->delete(true)withineach_by_idis concise and takes advantage of PHP 8's arrow functions. Ensure that thedeletemethod onQueue_Recordaccepts a boolean parameter and that its behavior is well-documented, especially what passingtruesignifies. - There is no error handling in the
handlemethod. Consider what should happen if the deletion fails for one or moreQueue_Recordinstances. Should the command fail entirely, log an error, or continue with the next record? This depends on the desired behavior and robustness of the command. - The command retrieves the
delete_afterconfiguration value. Ensure that this configuration option is documented and validated elsewhere in the codebase.
Here's a potential update to the take method if -1 is not intended:
- ->take(-1)
+ ->take($this->container['config']->get('queue.cleanup_batch_size', 100))This change assumes that there's a configuration setting named queue.cleanup_batch_size that controls how many records should be processed at a time.
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| <?php | |
| /** | |
| * Cleanup_Jobs_Commands class file. | |
| * | |
| * @package Mantle | |
| */ | |
| namespace Mantle\Queue\Console; | |
| use Mantle\Console\Command; | |
| use Mantle\Contracts\Container; | |
| use Mantle\Queue\Events\Job_Processed; | |
| use Mantle\Queue\Events\Job_Processing; | |
| use Mantle\Queue\Events\Run_Complete; | |
| use Mantle\Queue\Events\Run_Start; | |
| use Mantle\Queue\Providers\WordPress\Post_Status; | |
| use Mantle\Queue\Providers\WordPress\Queue_Record; | |
| /** | |
| * Queue Cleanup Command | |
| */ | |
| class Cleanup_Jobs_Command extends Command { | |
| /** | |
| * The console command name. | |
| * | |
| * @var string | |
| */ | |
| protected $name = 'queue:cleanup'; | |
| /** | |
| * Command Description. | |
| * | |
| * @var string | |
| */ | |
| protected $description = 'Cleanup old queue jobs.'; | |
| /** | |
| * Command action. | |
| */ | |
| public function handle() { | |
| Queue_Record::query() | |
| ->whereStatus( [ Post_Status::FAILED->value, Post_Status::COMPLETED->value ] ) | |
| ->olderThan( now()->subSeconds( (int) $this->container['config']->get( 'queue.delete_after', 60 ) ) ) | |
| ->take( -1 ) | |
| ->each_by_id( | |
| fn ( Queue_Record $record ) => $record->delete( true ), | |
| 100, | |
| ); | |
| } | |
| /** | |
| * Command action. | |
| */ | |
| public function handle() { | |
| Queue_Record::query() | |
| ->whereStatus( [ Post_Status::FAILED->value, Post_Status::COMPLETED->value ] ) | |
| ->olderThan( now()->subSeconds( (int) $this->container['config']->get( 'queue.delete_after', 60 ) ) ) | |
| ->take($this->container['config']->get('queue.cleanup_batch_size', 100)) | |
| ->each_by_id( | |
| fn ( Queue_Record $record ) => $record->delete( true ), | |
| 100, | |
| ); | |
| } |
srtfisher
added a commit
that referenced
this pull request
Dec 1, 2023
* Slight refactor for readability * Refactor to queue model * Use the query builder internally * WIP * Add phpstan typings * Continued work * Starting out on count() * Adding query builder count() method * Todo for tomorrow * Set the meta when creating the job * Improving the logic * Wrapping up until tmw * Continued work * Use the date query builder to get the proper queue items * Adjust cron logic to properly scale up to meet demand of the queue * Adding tests for cron queue * Linting fixes * Wrapping up fixes for queue * Raise memory limit, skip queue tests on 8.0 * Prevent queue service provider on 8.1 * Remove bogus version * Fixing tests * Remove bogus return * Add a log of queue job events * Filling in the queue log * Adding a database collection and passing along the found_rows to the collection * Allow the value to be passed * Queue UI (#458) * Continued work on queue UI= * Wrapping up the core of the queue UI * Working table for queue UI * Adding counts to the queue * Switch to use wp_count_posts for performance * Style the row table * Adding single view for queue job * Adding queue UI with fixes for retrying * Preserve queue job items and cleanup after some time (#472) * Continued work on queue UI= * Wrapping up the core of the queue UI * Working table for queue UI * Adding counts to the queue * Switch to use wp_count_posts for performance * Style the row table * Adding single view for queue job * Adding queue UI with fixes for retrying * Serialize enums when asserting against them * Keep queue job around after it is complete for logging * Fixing tests and adjusting comment * Fixing linting * Stubbing the cleanup command * Cleanup the processed/failed jobs after some time * Fixing phpcs issue * Remove checks for <8.1 now that were on PHP 8.1+ always * [1.x] Allow queue jobs to be run in-admin, allow delays for closure jobs (#481) * Add support for displaying completed queue job status * Allow a closure job to be delayed * Allow queue jobs to be run in-admin, allow closure jobs to be delayed * Add a back link on the queue job page * Cleanup running jobs with the cleanup command * Remove the superfluous take()
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.
Keep processed and failed jobs around for review after they are out of the system. After some time, come back and delete them using the application scheduler.
Also breaks out the queue model to
Queue_Recordto make it less confusing.Summary by CodeRabbit
New Features
Style
Documentation