Skip to content

[ISSUE #10405] Route orderly retry messages to topic brokers#10624

Open
btlqql wants to merge 1 commit into
apache:developfrom
btlqql:agent/fix-orderly-retry-routing
Open

[ISSUE #10405] Route orderly retry messages to topic brokers#10624
btlqql wants to merge 1 commit into
apache:developfrom
btlqql:agent/fix-orderly-retry-routing

Conversation

@btlqql

@btlqql btlqql commented Jul 18, 2026

Copy link
Copy Markdown

Which Issue(s) This PR Fixes

Brief Description

When an orderly consumer reaches maxReconsumeTimes, both the regular and POP orderly services publish the retry message with the default producer. Because the %RETRY% topic may not have publish route data yet, producer fallback can select a broker from TBW102 that does not host the original business topic. This creates retry topics on unrelated brokers and makes retry-topic placement inconsistent with the consumed topic.

This change:

  • adds one shared OrderlyRetryMessageSender for both orderly consumption paths;
  • resolves a snapshot of the business topic route, including PROPERTY_RETRY_TOPIC when a retry message is consumed again;
  • prefers the broker that supplied the original message, then fails over only to other brokers in the business topic route;
  • uses a fresh message and property copy for every send attempt so producer mutation cannot leak into later attempts or the consumed message;
  • preserves thread interruption and propagates the final targeted-send failure instead of falling back to an unrelated broker;
  • retains the previous default-producer behavior only when business topic route data is unavailable.

The change is internal to the orderly retry path and introduces no public API changes.

How Did You Test This Change?

mvn -o -B -ntp -pl client -am -Dtest=OrderlyRetryMessageSenderTest,ConsumeMessageOrderlyServiceTest,ConsumeMessagePopOrderlyServiceTest -Dsurefire.failIfNoSpecifiedTests=false test

Result: BUILD SUCCESS; 27 tests run, 0 failures, 0 errors, 0 skipped. The Maven lifecycle also completed Checkstyle with 0 violations and SpotBugs with 0 findings for the affected reactor modules.

The new tests cover original-broker preference, current-route filtering, cross-broker failover with fresh message copies, retry-message business-topic lookup, unavailable/empty route fallback, refusal to leave a known topic route, and interruption handling.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by github-manager-bot

LGTM. Well-designed fix for orderly retry message routing.

Summary

Fixes #10405 by routing orderly retry messages to the original broker first (via RECONSUME_WHEN), with fallback to other brokers on the same topic. Also fixes a property mutation bug by using deepCopyProperties instead of direct reference sharing.

Key Changes

  • OrderlyRetryMessageSender — New class that implements smart retry routing: prefers the original broker, falls back to other topic brokers. Thread interruption handling is correct.
  • deepCopyProperties — Fixes a subtle bug where clearProperty on the retry message would mutate the original message's properties map (shared reference).
  • ConsumeMessagePopOrderlyService — Same deepCopyProperties fix applied consistently.

Positive Notes

  • Comprehensive test coverage including edge cases (single broker, all brokers failing, interrupted thread)
  • Clean separation of concerns with the new OrderlyRetryMessageSender class
  • RETRY_QUEUE_ID = 0 is correct since broker-created retry topics always use queue 0
  • Consistent fix applied to both push and pop orderly services

Automated review by github-manager-bot

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by github-manager-bot

Summary

Fixes orderly consumption retry message routing by introducing OrderlyRetryMessageSender, which sends retry messages to the original topic's brokers (preferring the original broker) instead of blindly sending to the default producer target. Also uses deepCopyProperties to prevent shared mutable state.

Findings

[Info] Core fix — The OrderlyRetryMessageSender correctly:

  1. Looks up the original topic's broker names from the topic's route info
  2. Prefers the original broker (where the message came from)
  3. Falls back to other topic brokers in sorted order for determinism
  4. Falls back to default send if no broker info is available

[Info] Properties deep copy — Changing from setProperties(newMsg, msg.getProperties()) to setProperties(newMsg, MessageAccessor.deepCopyProperties(msg.getProperties())) fixes a latent bug where the retry message and original message shared the same properties map, risking mutation side effects.

[Info] Both ConsumeMessageOrderlyService and ConsumeMessagePopOrderlyService are updated consistently.

[Warning] Route fetch in retry path — findTopicBrokerNames() calls fetchSubscribeMessageQueues() which makes a network call to the NameServer. In the retry path (which is already error-handling), this adds latency and a potential failure point. The fallback to Collections.emptyList() (which triggers default send) is correct, but consider caching the route info briefly to avoid repeated NameServer lookups under high retry rates.

[Info] The RETRY_QUEUE_ID = 0 assumption is documented as "a retry topic created by the broker always has queue 0". This is correct for the current Broker implementation.

Suggestions

  • Consider adding a unit test for OrderlyRetryMessageSender that verifies the broker selection logic (prefer original, fallback to sorted others, empty fallback).
  • The copyMessage() helper uses MessageAccessor.cloneMessage() followed by deepCopyProperties(). Verify that cloneMessage() doesn't already deep-copy properties (which would make the second copy redundant).

Automated review by github-manager-bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants