RockteMQ-AI commented on PR #482: URL: https://github.com/apache/rocketmq-connect/pull/482#issuecomment-5506875953
Hi @odbozhou 👋 This PR has been open since April 2023. A quick review of the changes shows reasonable improvements: - **Task count calculation**: Using `Math.min(curMessageQueues.size(), maxTasks)` instead of always allocating `maxTasks` is a valid optimization. - **Lifecycle fix**: Moving `initAdmin()` from `taskConfigs()` to `start()` is more correct. - **Cleanup**: Adding `closeScheduleTask()` for proper resource cleanup is good practice. - **Thread safety**: Changing `curMessageQueues` to `volatile Set<MessageQueue>` addresses potential concurrency issues. However, the PR description still contains placeholder text (`XXXXX`). Could you: 1. Update the PR description with actual details about the changes 2. Confirm this is still relevant to the current codebase (it's been 2+ years) If the changes are still applicable, I'd be happy to do a more thorough review. --- *Automated notification by github-manager-bot* -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
