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/08/03 07:47:29 UTC
[PR] [ISSUE #10786] Tolerate malformed local POP offset metadata [rocketmq]
Aias00 opened a new pull request, #10787:
URL: https://github.com/apache/rocketmq/pull/10787
### What is changed
- Guard local POP receipt-handle reconstruction against missing `startOffsetInfo` / `msgOffsetInfo` entries and invalid offset indexes.
- Skip only the malformed message entry instead of failing the whole local POP response translation.
- Add `LocalMessageServiceTest` coverage for a POP response with `startOffsetInfo` present but missing `msgOffsetInfo`.
Fixes #10786
### Notes
- This intentionally does not change the existing raw `MessageExt` warning covered by #10730 / #10731.
### Verification
- mvn -pl proxy -Dtest=LocalMessageServiceTest 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 #10786] Tolerate malformed local POP offset metadata [rocketmq]
Posted by "RockteMQ-AI (via GitHub)" <gi...@apache.org>.
RockteMQ-AI commented on code in PR #10787:
URL: https://github.com/apache/rocketmq/pull/10787#discussion_r3720203078
##########
proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalMessageService.java:
##########
@@ -272,6 +272,11 @@ public CompletableFuture<PopResult> popMessage(ProxyContext ctx, AddressableMess
sortMap.get(key).add(messageExt.getQueueOffset());
}
Map<String, String> map = new HashMap<>(5);
+ List<MessageExt> validMessageExtList = new ArrayList<>(messageExtList.size());
Review Comment:
Raw type usage detected. Use parameterized types for type safety.
--
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 #10786] Tolerate malformed local POP offset metadata [rocketmq]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 closed pull request #10787: [ISSUE #10786] Tolerate malformed local POP offset metadata
URL: https://github.com/apache/rocketmq/pull/10787
--
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 #10786] Tolerate malformed local POP offset metadata [rocketmq]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #10787:
URL: https://github.com/apache/rocketmq/pull/10787#issuecomment-5293902773
Rebased onto the latest develop and verified the review fixes already included in the branch: malformed POP metadata is summarized once per response, valid-message filtering preserves the status/list invariant, and targeted tests cover missing start-offset and invalid offset-index cases.
Verified with:
mvn -q -pl proxy -am -Dtest=LocalMessageServiceTest -Dsurefire.failIfNoSpecifiedTests=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 #10786] Tolerate malformed local POP offset metadata [rocketmq]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #10787:
URL: https://github.com/apache/rocketmq/pull/10787#discussion_r3702242593
##########
proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalMessageService.java:
##########
@@ -306,7 +319,9 @@ public CompletableFuture<PopResult> popMessage(ProxyContext ctx, AddressableMess
messageExt.getProperties().computeIfAbsent(MessageConst.PROPERTY_FIRST_POP_TIME, k -> String.valueOf(responseHeader.getPopTime()));
messageExt.setBrokerName(messageQueue.getBrokerName());
messageExt.setTopic(messageQueue.getTopic());
+ validMessageExtList.add(messageExt);
}
+ popResult.setMsgFoundList(validMessageExtList);
Review Comment:
After filtering malformed entries, `popResult` can end up with `PopStatus.FOUND` but an empty `msgFoundList` (the new test asserts this). This is an inconsistent API signal for callers that treat FOUND as 'messages returned' and can cause tight polling loops. Consider updating the status (e.g., to `NO_NEW_MSG`) when `validMessageExtList` is empty, or documenting/enforcing an invariant between status and message list.
##########
proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalMessageService.java:
##########
@@ -285,14 +286,26 @@ public CompletableFuture<PopResult> popMessage(ProxyContext ctx, AddressableMess
} else {
if (messageExt.getProperty(MessageConst.PROPERTY_POP_CK) == null) {
String key = ExtraInfoUtil.getStartOffsetInfoMapKey(messageExt.getTopic(), messageExt.getQueueId());
- int index = sortMap.get(key).indexOf(messageExt.getQueueOffset());
- Long msgQueueOffset = msgOffsetInfo.get(key).get(index);
+ List<Long> sortQueueOffsets = sortMap.get(key);
+ List<Long> msgQueueOffsets = msgOffsetInfo == null ? null : msgOffsetInfo.get(key);
+ Long startOffset = startOffsetInfo.get(key);
Review Comment:
The local variable name `startOffset` is ambiguous here (it can be confused with other start offsets in POP flow or outer-scope variables). Renaming to something keyed (e.g., `startOffsetForQueue` / `startOffsetForKey`) would make the metadata usage clearer and reduce maintenance risk.
##########
proxy/src/test/java/org/apache/rocketmq/proxy/service/message/LocalMessageServiceTest.java:
##########
@@ -340,6 +340,47 @@ public void testPopMessageWriteAndFlush() throws Exception {
}
}
+ @Test
+ public void testPopMessageShouldSkipMessageWithMissingOffsetMetadata() throws Exception {
Review Comment:
This test covers missing `msgOffsetInfo`, but the production changes also handle invalid offset indexes (`index < 0` / `index >= msgQueueOffsets.size()`) and missing `startOffsetInfo` entries for a key. Adding targeted tests for those branches would prevent regressions in the new guard logic.
##########
proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalMessageService.java:
##########
@@ -285,14 +286,26 @@ public CompletableFuture<PopResult> popMessage(ProxyContext ctx, AddressableMess
} else {
if (messageExt.getProperty(MessageConst.PROPERTY_POP_CK) == null) {
String key = ExtraInfoUtil.getStartOffsetInfoMapKey(messageExt.getTopic(), messageExt.getQueueId());
- int index = sortMap.get(key).indexOf(messageExt.getQueueOffset());
- Long msgQueueOffset = msgOffsetInfo.get(key).get(index);
+ List<Long> sortQueueOffsets = sortMap.get(key);
+ List<Long> msgQueueOffsets = msgOffsetInfo == null ? null : msgOffsetInfo.get(key);
+ Long startOffset = startOffsetInfo.get(key);
+ if (sortQueueOffsets == null || msgQueueOffsets == null || startOffset == null) {
+ log.warn("Pop response offset metadata is missing, key:{}", key);
+ continue;
+ }
+ int index = sortQueueOffsets.indexOf(messageExt.getQueueOffset());
+ if (index < 0 || index >= msgQueueOffsets.size()) {
+ log.warn("Pop response offset metadata index is invalid, key:{}, index:{}, msgOffsetCount:{}",
+ key, index, msgQueueOffsets.size());
+ continue;
+ }
Review Comment:
These `warn` logs execute per message and could become noisy under persistent malformed metadata (especially with batch POP). Consider aggregating (log once per response with counts/first key), rate-limiting, or downgrading to `info/debug` while keeping enough context to diagnose.
--
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 #10786] Tolerate malformed local POP offset metadata [rocketmq]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10787:
URL: https://github.com/apache/rocketmq/pull/10787#issuecomment-5164146296
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/10787?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 `68.75000%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 48.23%. Comparing base ([`eddb235`](https://app.codecov.io/gh/apache/rocketmq/commit/eddb235c1bd8adc5c3a1125a294405526e7bb0e3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b83e99a`](https://app.codecov.io/gh/apache/rocketmq/commit/b83e99ae6c2b665a323354bb4636589ef635bd40?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/10787?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...tmq/proxy/service/message/LocalMessageService.java](https://app.codecov.io/gh/apache/rocketmq/pull/10787?src=pr&el=tree&filepath=proxy%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fproxy%2Fservice%2Fmessage%2FLocalMessageService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHJveHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Byb3h5L3NlcnZpY2UvbWVzc2FnZS9Mb2NhbE1lc3NhZ2VTZXJ2aWNlLmphdmE=) | 68.75% | [3 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/10787?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 #10787 +/- ##
=============================================
- Coverage 48.24% 48.23% -0.01%
+ Complexity 13496 13489 -7
=============================================
Files 1380 1380
Lines 101104 101117 +13
Branches 13107 13110 +3
=============================================
+ Hits 48773 48774 +1
- Misses 46350 46356 +6
- Partials 5981 5987 +6
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/rocketmq/pull/10787?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 #10786] Tolerate malformed local POP offset metadata [rocketmq]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #10787:
URL: https://github.com/apache/rocketmq/pull/10787#issuecomment-5301174677
Consolidated into #10731; both changes harden LocalMessageService POP scans and diagnostics.
--
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