[ISSUE #10786] Tolerate malformed local POP offset metadata - #10787
[ISSUE #10786] Tolerate malformed local POP offset metadata#10787Aias00 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Improve robustness of local POP response translation by tolerating malformed offset metadata, skipping only invalid message entries, and adding regression coverage for missing msgOffsetInfo.
Changes:
- Guard POP receipt-handle reconstruction against missing
startOffsetInfo/msgOffsetInfoand invalid offset indexes. - Skip only malformed messages and return remaining valid messages.
- Add a unit test for POP responses with
startOffsetInfopresent but missingmsgOffsetInfo.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalMessageService.java | Adds defensive checks for offset metadata and filters invalid messages instead of failing translation. |
| proxy/src/test/java/org/apache/rocketmq/proxy/service/message/LocalMessageServiceTest.java | Adds regression test ensuring messages are skipped when msgOffsetInfo is missing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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); |
| 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; | ||
| } |
| @Test | ||
| public void testPopMessageShouldSkipMessageWithMissingOffsetMetadata() throws Exception { |
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Adds null-safe checks for POP offset metadata (sortQueueOffsets, msgQueueOffsets, startOffset) in LocalMessageService. Messages with missing/malformed metadata are now skipped with a warning instead of causing NPE.
Findings
- [Info]
LocalMessageService.java:432-435— The three-way null check (sortQueueOffsets, msgQueueOffsets, startOffset) correctly guards against all NPE paths in the original code. - [Info] The
validMessageExtListpattern is clean — only messages passing all validation are included in the result. - [Info] Test
testPopLocalMessage_MalformedOffsetMetadata_SkipsMessageproperly verifies the skip behavior.
Suggestions
- Minor: the
msgQueueOffsets == null ? null : msgQueueOffsets.get(key)pattern could be simplified withOptionalor a helper, but this is a style preference and not blocking.
LGTM — solid defensive fix against NPE in POP message processing.
Automated review by github-manager-bot
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10787 +/- ##
=============================================
- Coverage 48.34% 48.27% -0.08%
+ Complexity 13527 13507 -20
=============================================
Files 1380 1380
Lines 101104 101164 +60
Branches 13107 13128 +21
=============================================
- Hits 48882 48834 -48
- Misses 46267 46343 +76
- Partials 5955 5987 +32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR looks mostly good with some suggestions for improvement.
Findings Overview
- 1 warning(s) and suggestions for improvement
Please review the inline comments.
Automated review by github-manager-bot
| sortMap.get(key).add(messageExt.getQueueOffset()); | ||
| } | ||
| Map<String, String> map = new HashMap<>(5); | ||
| List<MessageExt> validMessageExtList = new ArrayList<>(messageExtList.size()); |
There was a problem hiding this comment.
Raw type usage detected. Use parameterized types for type safety.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Improved tolerance for malformed POP offset metadata. The second commit significantly improves the implementation:
- Batch logging — counts skipped messages and logs once per category instead of per-message, reducing log noise under load.
NO_NEW_MSGstatus — correctly sets the status when all messages are filtered out, preventing unnecessary polling retries.- Test refactoring —
mockPopMessageResponse()helper eliminates duplication across test cases. - Additional test coverage — new tests for missing start offset and invalid offset index scenarios.
The raw type concern from the previous review appears to be about pre-existing code, not the new changes. All new code uses proper parameterized types. LGTM.
Automated review by github-manager-bot
What is changed
startOffsetInfo/msgOffsetInfoentries and invalid offset indexes.LocalMessageServiceTestcoverage for a POP response withstartOffsetInfopresent but missingmsgOffsetInfo.Fixes #10786
Notes
MessageExtwarning covered by Avoid logging raw MessageExt bodies in LocalMessageService #10730 / [ISSUE #10730] Avoid raw local POP message logs #10731.Verification