[ISSUE #10786] Tolerate malformed local POP offset metadata - #10787
[ISSUE #10786] Tolerate malformed local POP offset metadata#10787Aias00 wants to merge 1 commit 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.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 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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