You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "Aias00 (via GitHub)" <gi...@apache.org> on 2026/07/29 07:59:22 UTC
[PR] [ISSUE #10688] Skip malformed ordered route items [rocketmq]
Aias00 opened a new pull request, #10689:
URL: https://github.com/apache/rocketmq/pull/10689
### Which Issue(s) This PR Fixes
Fixes #10688
### Brief Description
`MessageQueueSelector.buildWrite` previously assumed every ordered route fragment from `orderTopicConf` used a valid `brokerName:queueNum` format. Malformed fragments could throw `ArrayIndexOutOfBoundsException` or `NumberFormatException`, and unknown broker names could make `TopicRouteWrapper.getMasterAddr` throw before the selector's null check could skip them.
This PR validates each ordered route fragment before using it, skips malformed entries with a warning, and makes `TopicRouteWrapper.getMasterAddr` return `null` for unknown brokers so existing skip logic works as intended. Valid ordered route entries keep the existing behavior.
### How Did You Test This Change?
```bash
mvn -pl proxy -Dtest=MessageQueueSelectorTest -DfailIfNoTests=false test
```
Result: `BUILD SUCCESS`, `Tests run: 3, Failures: 0, Errors: 0, Skipped: 0`.
--
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
Re: [PR] [ISSUE #10688] Skip malformed ordered route items [rocketmq]
Posted by "fuyou001 (via GitHub)" <gi...@apache.org>.
fuyou001 commented on PR #10689:
URL: https://github.com/apache/rocketmq/pull/10689#issuecomment-5116882868
Suggestion: consider changing `TopicRouteWrapper.getMasterAddr` to return `Optional<String>` instead of a nullable `String`. There are only three direct call sites, and `Optional` would force each one to handle a missing broker explicitly.
- In the ordered-route branch of `MessageQueueSelector.buildWrite`, use an empty result to log and skip the malformed/unknown broker entry.
- In the normal `QueueData` write branch, use an empty result to skip entries without a master address.
- In `ClusterTopicRouteService.getBrokerAddr`, convert an empty result into the existing explicit broker-not-found exception rather than returning `null`.
The third point is important because `ClusterTopicRouteService.getBrokerAddr` has many indirect callers. Returning `null` can bypass the exception handling in `resolveBrokerAddr` and `resolveBrokerAddrInReceiptHandle`, allowing a null address to reach the remoting client instead of producing `INVALID_BROKER_NAME` or `INVALID_RECEIPT_HANDLE`.
Please also add regression tests asserting that an unknown broker produces `Optional.empty()`, while `ClusterTopicRouteService.getBrokerAddr` still fails with the expected domain exception.
--
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
Re: [PR] [ISSUE #10688] Skip malformed ordered route items [rocketmq]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10689:
URL: https://github.com/apache/rocketmq/pull/10689#issuecomment-5115992267
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/10689?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `85.71429%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 48.25%. Comparing base ([`00e45b8`](https://app.codecov.io/gh/apache/rocketmq/commit/00e45b8a6db23efbe756d0306f10716156cfd4dd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9bdec74`](https://app.codecov.io/gh/apache/rocketmq/commit/9bdec747378d00c63c5d90084b5af9c5a7aae430?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/rocketmq/pull/10689?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...etmq/proxy/service/route/MessageQueueSelector.java](https://app.codecov.io/gh/apache/rocketmq/pull/10689?src=pr&el=tree&filepath=proxy%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fproxy%2Fservice%2Froute%2FMessageQueueSelector.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHJveHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Byb3h5L3NlcnZpY2Uvcm91dGUvTWVzc2FnZVF1ZXVlU2VsZWN0b3IuamF2YQ==) | 90.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/10689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ocketmq/proxy/service/route/TopicRouteWrapper.java](https://app.codecov.io/gh/apache/rocketmq/pull/10689?src=pr&el=tree&filepath=proxy%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fproxy%2Fservice%2Froute%2FTopicRouteWrapper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHJveHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Byb3h5L3NlcnZpY2Uvcm91dGUvVG9waWNSb3V0ZVdyYXBwZXIuamF2YQ==) | 75.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/10689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## develop #10689 +/- ##
=============================================
- Coverage 48.31% 48.25% -0.07%
+ Complexity 13511 13491 -20
=============================================
Files 1380 1380
Lines 101091 101103 +12
Branches 13101 13103 +2
=============================================
- Hits 48844 48783 -61
- Misses 46285 46339 +54
- Partials 5962 5981 +19
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/rocketmq/pull/10689?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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
Re: [PR] [ISSUE #10688] Skip malformed ordered route items [rocketmq]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #10689:
URL: https://github.com/apache/rocketmq/pull/10689#issuecomment-5138009404
Added one more regression commit for the exceptional future propagation part of the review feedback.
New coverage in `ClusterMessageServiceTest` asserts that broker lookup failures complete returned futures exceptionally for both paths:
- `ClusterMessageService.request(...)`
- `ClusterMessageService.requestOneway(...)`
Both tests verify the propagated `ProxyExceptionCode.INVALID_BROKER_NAME`, so the invalid broker-name path does not proceed to remoting with a missing address.
Local verification with JDK 8:
`JAVA_HOME=$(/usr/libexec/java_home -v 1.8) mvn -pl proxy -Dtest=MessageQueueSelectorTest,ClusterTopicRouteServiceTest,ClusterMessageServiceTest -DfailIfNoTests=false test`
Result: `Tests run: 14, Failures: 0, Errors: 0, Skipped: 0`; checkstyle and spotbugs also passed in the same run.
--
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
Re: [PR] [ISSUE #10688] Skip malformed ordered route items [rocketmq]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #10689:
URL: https://github.com/apache/rocketmq/pull/10689#issuecomment-5137983308
Addressed the latest feedback in commit `77c9853`:\n\n- Restored `TopicRouteWrapper.getMasterAddr(String)` with its original nullable `String` signature to preserve source/binary compatibility. Added `getOptionalMasterAddr(String)` for the new explicit missing-broker handling.\n- Updated the new call sites to use `getOptionalMasterAddr` while keeping existing callers compatible.\n- Added ordered-topic queue count validation: only `1..1024` is accepted. This covers zero, negative, `Integer.MAX_VALUE`, and integer overflow values without allocating large queue lists.\n- Added regression coverage for zero, negative, max-int, overflow, all-items-invalid, and missing-master ordered route cases.\n\nFor the consistency policy: this PR intentionally keeps the existing defensive behavior of building a view from valid route fragments and skipping malformed fragments. Fully rejecting a refreshed ordered-topic route and retaining the last valid view would require a cache-level policy change i
n `TopicRouteService`, which is broader than this small parsing/validation fix and probably should be discussed separately.\n\nLocal verification passed:\n`mvn -pl proxy -Dtest=MessageQueueSelectorTest,ClusterTopicRouteServiceTest -DfailIfNoTests=false test`
--
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
Re: [PR] [ISSUE #10688] Skip malformed ordered route items [rocketmq]
Posted by "fuyou001 (via GitHub)" <gi...@apache.org>.
fuyou001 commented on PR #10689:
URL: https://github.com/apache/rocketmq/pull/10689#issuecomment-5128558419
The validation direction looks useful, but I think a few compatibility and resource-safety cases should be addressed before merging.
1. `TopicRouteWrapper.getMasterAddr` changes its public return type from `String` to `Optional<String>`. This is source- and binary-incompatible for external extensions or code compiled against the proxy module. Please consider retaining the existing signature and adding a separate safe lookup method, unless this type is explicitly guaranteed not to be an extension API.
2. The queue count is only checked for integer syntax. A negative value silently produces no queues, while a very large positive value can allocate a large number of `AddressableMessageQueue` objects during route refresh. Please require a positive count and enforce a reasonable upper bound based on RocketMQ queue limits.
3. Partially accepting a damaged ordered-topic configuration changes the ordered queue set. Please confirm that partial acceptance is the desired consistency policy; otherwise, rejecting the whole new configuration and retaining the last valid route would avoid different nodes constructing different queue views during mixed-version operation.
Please add regression coverage for zero, negative, `Integer.MAX_VALUE`, overflow, all-items-invalid, and missing-master cases; route refresh/cache recovery with a previously valid route; and mixed old/new proxy behavior. Please also assert how the new `ProxyException` from `ClusterTopicRouteService.getBrokerAddr` is mapped and propagated through `ConsumerProcessor`, `ClusterMessageService`, `buildAddressableMessageQueue`, and the background fault-strategy callback, including exceptional future completion.
--
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
Re: [PR] [ISSUE #10688] Skip malformed ordered route items [rocketmq]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #10689:
URL: https://github.com/apache/rocketmq/pull/10689#issuecomment-5117885100
Addressed the review suggestion in the latest commit:\n\n- Changed `TopicRouteWrapper.getMasterAddr` to return `Optional<String>`.\n- Updated `MessageQueueSelector.buildWrite` to explicitly skip route entries without a master broker address.\n- Updated `ClusterTopicRouteService.getBrokerAddr` to convert a missing master broker address into `ProxyExceptionCode.INVALID_BROKER_NAME` instead of returning `null`.\n- Added regression coverage for unknown broker `Optional.empty()`, normal queue-data skip behavior, and the ClusterTopicRouteService domain exception.\n\nLocal verification passed:\n`mvn -pl proxy -Dtest=MessageQueueSelectorTest,ClusterTopicRouteServiceTest -DfailIfNoTests=false test`
--
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