RockteMQ-AI commented on PR #10408: URL: https://github.com/apache/rocketmq/pull/10408#issuecomment-4691775725
## Review by github-manager-bot ### Summary Fixes orderly consumer retry message routing by sending to brokers that host the original topic instead of falling back to TBW102, which routes to all brokers. ### Findings - **[Warning]** ConsumeMessageOrderlyService.java:414 and ConsumeMessagePopOrderlyService.java:328 — `trySendToTopicBroker` modifies `retryMsg` state on each iteration: `send(retryMsg, retryMQ)` may internally set properties on the message (e.g., `setDelayTimeLevel` or other fields). If the first broker send fails and the loop retries on a second broker, the message object may be in a partially-modified state from the failed attempt. This depends on the producer implementation but is worth verifying. - **[Warning]** ConsumeMessageOrderlyService.java:406 and ConsumeMessagePopOrderlyService.java:320 — The `table` reference is obtained once but `getTopicSubscribeInfoTable()` returns a live concurrent map. The entry for `originalMsg.getTopic()` could be removed between the null/empty check and iteration. This is a minor race — the worst case is a `ConcurrentModificationException` or empty iteration, which would fall through to the default send. Low risk but worth noting. - **[Info]** ConsumeMessageOrderlyService.java:420 and ConsumeMessagePopOrderlyService.java:334 — `new MessageQueue(retryMsg.getTopic(), brokerName, 0)` uses queueId `0`. For retry topics, this is fine since retry topics typically have a single queue, but the hardcoded `0` could be confusing. A comment or constant would clarify intent. - **[Info]** Both files — The `trySendToTopicBroker` method is duplicated verbatim across the two classes. Consider extracting it to a shared utility or a base class to reduce code duplication. - **[Info]** Both files — No unit tests are included. The PR description lists test plan items as unchecked. Given the routing logic is safety-critical for message delivery, tests covering: (1) original broker preferred, (2) failover to other broker, (3) empty route fallback, are strongly recommended. ### Suggestions 1. Verify that `DefaultMQProducer.send(msg, mq)` doesn't mutate the `Message` object in a way that affects retries on subsequent brokers. If it does, consider creating a fresh copy per attempt or resetting any modified state. 2. Extract the identical `trySendToTopicBroker` into a shared method (e.g., in a utility class or via a common base) to avoid the exact duplication across the two service classes. 3. Add unit tests — at minimum, verify the broker ordering preference and fallback behavior. The logic is subtle and hard to verify manually in integration. --- *Automated review 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]
