You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@rocketmq.apache.org by "RockteMQ-AI (via GitHub)" <gi...@apache.org> on 2026/09/02 08:41:07 UTC

Re: [PR] [ISSUES #481 ] Support requestTaskReconfiguration [rocketmq-connect]

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: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org