Add cleanup command #119
Add cleanup command #119
Conversation
|
Warning Rate limit exceeded@wannevancamp has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughA new asynchronous cleanup mechanism for mail archive entries is introduced. The changes add a Symfony console command, a Messenger message and handler, and refactor the scheduled cleanup task to dispatch the cleanup as a message instead of executing it synchronously. The actual cleanup logic is now handled by the new message handler. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConsoleCommand as CleanupCommand
participant MessageBus
participant Handler as CleanupHandler
participant Database
participant FileManager as EmlFileManager
User->>ConsoleCommand: Execute 'frosh:mail-archive:cleanup'
ConsoleCommand->>MessageBus: Dispatch CleanupMessage
MessageBus->>Handler: Deliver CleanupMessage
Handler->>Database: Query for old mail archive entries
Handler->>FileManager: Delete associated EML files
Handler->>Database: Delete old mail archive records
sequenceDiagram
participant ScheduledTask
participant TaskHandler as CleanupTaskHandler
participant MessageBus
participant Handler as CleanupHandler
ScheduledTask->>TaskHandler: Trigger run()
TaskHandler->>MessageBus: Dispatch CleanupMessage
MessageBus->>Handler: Deliver CleanupMessage
Handler->>...: (Performs cleanup as above)
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/Command/CleanupCommand.php (1)
24-33: Consider clarifying the asynchronous nature of the cleanup.The command returns success immediately after dispatching the message, which is correct for async processing. However, consider updating the output messages to clarify that the cleanup will be processed asynchronously.
- $output->writeln('Dispatching mail archive cleanup...'); + $output->writeln('Dispatching mail archive cleanup (will be processed asynchronously)...'); $this->messageBus->dispatch(new CleanupMessage()); - $output->writeln('Cleanup message dispatched successfully.'); + $output->writeln('Cleanup message dispatched successfully. Processing will continue in the background.');src/Messenger/CleanupHandler.php (2)
47-51: Consider batch processing for large datasets.Loading all records at once with
fetchAllAssociative()could consume significant memory for large datasets. Consider implementing batch processing for better memory efficiency.- $result = $query->executeQuery()->fetchAllAssociative(); - - if (\count($result) === 0) { - return; - } + $batchSize = 1000; + $offset = 0; + + do { + $batchQuery = clone $query; + $batchQuery->setFirstResult($offset); + $batchQuery->setMaxResults($batchSize); + + $result = $batchQuery->executeQuery()->fetchAllAssociative(); + + if (\count($result) === 0) { + break; + } + + // Process batch... + + $offset += $batchSize; + } while (\count($result) === $batchSize);
53-59: Consider logging file deletion failures.The current implementation silently continues if EML file deletion fails. Consider adding logging to track cleanup issues for monitoring purposes.
+use Psr\Log\LoggerInterface; public function __construct( private readonly SystemConfigService $configService, private readonly Connection $connection, private readonly EmlFileManager $emlFileManager, + private readonly LoggerInterface $logger, ) { } foreach ($result as $item) { if (empty($item['eml_path']) || !\is_string($item['eml_path'])) { continue; } - $this->emlFileManager->deleteEmlFile($item['eml_path']); + try { + $this->emlFileManager->deleteEmlFile($item['eml_path']); + } catch (\Throwable $e) { + $this->logger->warning('Failed to delete EML file during cleanup', [ + 'file_path' => $item['eml_path'], + 'error' => $e->getMessage() + ]); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Command/CleanupCommand.php(1 hunks)src/Messenger/CleanupHandler.php(1 hunks)src/Messenger/CleanupMessage.php(1 hunks)src/Task/CleanupTask.php(1 hunks)src/Task/CleanupTaskHandler.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Command/CleanupCommand.php (1)
src/Messenger/CleanupMessage.php (1)
CleanupMessage(9-12)
src/Messenger/CleanupHandler.php (2)
src/Services/EmlFileManager.php (2)
EmlFileManager(12-93)deleteEmlFile(85-92)src/Messenger/CleanupMessage.php (1)
CleanupMessage(9-12)
🔇 Additional comments (4)
src/Task/CleanupTask.php (1)
20-20: LGTM! Good clarification.The comment clearly explains that 86400 represents 1 day in seconds, improving code readability.
src/Messenger/CleanupMessage.php (1)
9-12: LGTM! Follows Symfony Messenger best practices.The empty message class is a valid pattern when no data needs to be passed between the dispatcher and handler.
src/Task/CleanupTaskHandler.php (2)
7-12: Clean refactoring to use message-driven architecture!The imports are appropriate for the new asynchronous cleanup approach. This separation of concerns will make the system more maintainable and testable.
20-25: Constructor properly simplified.The dependency injection is correctly implemented, replacing the previous direct dependencies with the message bus interface.
wannevancamp
force-pushed
the
cleanup-command
branch
from
352f821 to
1265b4e
Compare
wannevancamp
force-pushed
the
cleanup-command
branch
from
1265b4e to
a78fc6e
Compare
1 participant
Clean up mail archive entries via a console command, enabling crontab monitoring with tools like Oh Dear.
Summary by CodeRabbit
New Features
Refactor