[ISSUE #9633] Enable retry topic V2 by default to prevent name collision

[wang-jiahua]

Which Issue(s) This PR Fixes

Fixes #9633

Brief Description

V1 retry topic naming uses underscore as separator (%RETRY%{cid}_{topic}), which is ambiguous because both topic and group names may contain underscores. This causes different (group, topic) pairs to map to the same retry topic, leading to cross-topic consumption.

V2 naming (using + separator) already exists but was disabled by default. This PR changes enableRetryTopicV2 default from false to true.

Existing V1 retry topics are still readable because retrieveMessageFromPopRetryTopicV1 remains true (backward compatible).

While validating this default flip, CI exposed a pre-existing gap in the V1 compatibility window that only becomes visible once V2 is the default: when both enableRetryTopicV2 and retrieveMessageFromPopRetryTopicV1 are on, PopMessageProcessor picks V1 or V2 retry topic per request by randomQ % 2. In normal mode that alternation is fine (a miss is covered by the next request), but in priority mode (popFromRetryProbabilityForPriority) the retry read happens exactly once before the normal topic, so landing on the (often empty) V1 topic silently skips the V2 retry messages for that request — PopPriorityIT#test_priority_consume_retry_as_highest fails ~50% of the time. This PR fixes it by reading both naming schemes in priority mode while the compatibility window is open (coverBothRetryTopics), keeping the alternation unchanged for normal mode.

How Did You Test This Change

Unit tests: KeyBuilderTest (3 new tests demonstrating V1 collision and V2 correctness).

Integration tests: PopPriorityIT#test_priority_consume_retry_as_highest reproduced the priority-mode gap (~50% failure, expected:<1> but was:<0> on the first popped message) and passes 4/4 parameter combinations across repeated runs after the fix. PopMessageProcessorTest 8/8 and PopReviveServiceTest 12/12 under the V2 default.

[Copilot]

Pull request overview

This PR addresses retry-topic name collisions in POP retry topic V1 (%RETRY%{cid}_{topic}) by enabling the collision-free V2 naming scheme (%RETRY%{cid}+{topic}) by default in BrokerConfig, while keeping V1 retry-topic retrieval enabled for backward compatibility.

Changes:

• Switch BrokerConfig.enableRetryTopicV2 default from false to true.
• Add unit tests demonstrating a concrete V1 collision case and verifying V2 avoids it.
• Add a unit test asserting the new default for enableRetryTopicV2.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java	Enables retry topic V2 by default to avoid V1 name collisions.
common/src/test/java/org/apache/rocketmq/common/KeyBuilderTest.java	Adds tests covering V1 collision behavior, V2 non-collision, and the new default config value.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Changes enableRetryTopicV2 default from false to true, fixing the V1 retry topic naming collision where different (group, topic) pairs can produce the same retry topic name (e.g., group="A_B", topic="C" and group="A", topic="B_C" both yield %RETRY%A_B_C).

Findings

• [Info] BrokerConfig.java:240 — Single-line default change. V2 uses + separator which is not allowed in topic/group names, eliminating ambiguity.
• [Info] Backward compatibility is maintained: retrieveMessageFromPopRetryTopicV1 remains true, so existing V1 retry topics are still readable during and after upgrade.
• [Info] Tests are well-structured: testV1CollisionExample demonstrates the bug, testV2NoCollision proves the fix, testDefaultEnableRetryTopicV2IsTrue locks in the default.
• [Warning] KeyBuilderTest.java:89 — Missing newline at end of file (trailing \ No newline at end of file). Minor style issue.

Suggestions

• Rolling upgrade consideration: During a rolling upgrade, some brokers may use V1 naming while others use V2. New messages will be routed to V2 retry topics on upgraded brokers. Ensure that consumers on older broker versions can still process messages from V2 retry topics, or document this as a known limitation.
• Consider adding a release note / migration guide entry since this changes default behavior.

Overall: correct fix for a fundamental naming design flaw. The backward compatibility path is properly preserved.

---

Automated review by github-manager-bot

[qianye1001]

I don't think we should enable retry topic V2 by default in this PR yet.

Keeping retrieveMessageFromPopRetryTopicV1=true only proves that the new version can still read existing V1 retry topics. It does not fully validate a graceful upgrade path from V1 to V2, especially for rolling upgrades or mixed-version / mixed-config clusters where some brokers may still produce or consume retry messages using the V1 naming rule while others switch to V2.

Before changing the default, we should have explicit upgrade compatibility verification covering the V1 -> V2 transition, or keep enableRetryTopicV2 disabled by default and make the migration opt-in/documented.

[wang-jiahua]

Thanks for the feedback @qianye1001! You raise a valid point — enabling V2 by default is a significant behavior change.

Given your concern, I have two options:

1. Close this PR — if the V2 migration should be handled differently (e.g., per-topic migration, or a deprecation timeline)
2. Add a deprecation warning — keep V1 as default but log a WARN when a collision is detected, guiding users to migrate to V2

Which approach do you prefer? Or should I close this PR entirely?

[RockteMQ-AI]

Review by github-manager-bot (Re-review after new commit)

Summary

Re-reviewed after commit 743e72f (2026-07-01T05:46Z). Changes enableRetryTopicV2 default from false to true.

Assessment: ✅ Looks Good

• Correctness — V2 uses + separator which is not allowed in topic/group names, eliminating the V1 naming collision where group="A_B", topic="C" and group="A", topic="B_C" both produce %RETRY%A_B_C.
• Backward compatibility — retrieveMessageFromPopRetryTopicV1 remains true, so existing V1 retry topics are still readable during transition.
• Default change impact — New retry topics will use V2 naming. Existing V1 retry topics continue to work. This is the correct approach for fixing the collision without breaking existing deployments.
• Tests — Three new tests:
  • testV1CollisionExample — Demonstrates the collision problem
  • testV2NoCollision — Verifies V2 resolves it
  • testDefaultEnableRetryTopicV2IsTrue — Guards against accidental default regression

Clean, well-scoped change with appropriate test coverage.

[RockteMQ-AI]

Review by github-manager-bot (Re-review after new commit)

Changes since last review

No functional changes detected. The commit appears to be a rebase or amend with the same content.

Previous findings (unchanged)

• [Warning] KeyBuilderTest.java — Still missing newline at end of file. Trivial style issue, non-blocking.

Verdict

No new concerns. Previous review stands. LGTM 👍

---

Automated review by github-manager-bot

[qianye1001]

建议 close

[RockteMQ-AI]

Summary

Defensive fix with proper validation and test coverage. LGTM.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Summary

Re-review after new commits. The test updates improve robustness by reading enableRetryTopicV2 from config instead of hardcoding false, making tests adapt to future config changes. The core change (enabling V2 by default to fix name collision) remains sound.

Changes Since Last Review

• PopMessageProcessorTest: pins retry topic version via setRetrieveMessageFromPopRetryTopicV1(false) for test stability
• PopReviveServiceTest: uses brokerConfig.isEnableRetryTopicV2() instead of hardcoded false
• KeyBuilderTest: adds tests demonstrating V1 collision and V2 correctness

Assessment

The test improvements are reasonable and the core fix is still valid. V2 naming (+ separator) eliminates the ambiguity that caused cross-topic consumption.

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.51%. Comparing base (e348efa) to head (40d849d).

Files with missing lines	Patch %	Lines
...rocketmq/broker/processor/PopMessageProcessor.java	0.00%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10565      +/-   ##
=============================================
- Coverage      48.58%   48.51%   -0.07%     
+ Complexity     13678    13652      -26     
=============================================
  Files           1381     1381              
  Lines         101475   101477       +2     
  Branches       13190    13193       +3     
=============================================
- Hits           49304    49236      -68     
- Misses         46170    46222      +52     
- Partials        6001     6019      +18     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[RockteMQ-AI]

Summary

Fix for retry topic V2 default enablement. New commits add proper handling for priority mode where both V1 and V2 retry topics need to be covered simultaneously, preventing message loss during the compatibility window.

The coverBothRetryTopics logic correctly ensures that in priority mode with both V1/V2 enabled, messages are fetched from both retry topic naming schemes. Test fixes pin the retry topic version to avoid flaky behavior.

LGTM.

---

Automated review by RocketMQ-AI