[ISSUE #9195] Propagate MessageBatch user properties to inner messages#10588
[ISSUE #9195] Propagate MessageBatch user properties to inner messages#10588Aias00 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #9195 by ensuring that user properties set on a MessageBatch wrapper are propagated to all inner Message instances, so those properties are actually present in the encoded/decoded payload consumed by clients.
Changes:
- Override
MessageBatch.putUserProperty(...)to copy the property onto each inner message. - Add encode/decode tests to validate user-property behavior for
MessageBatch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| common/src/main/java/org/apache/rocketmq/common/message/MessageBatch.java | Propagates batch-level user properties onto inner messages during putUserProperty(...). |
| common/src/test/java/org/apache/rocketmq/common/MessageEncodeDecodeTest.java | Adds tests asserting inner-message user properties are present after batch encode/decode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MessageBatch messageBatch = MessageBatch.generateFromList(messages); | ||
| messageBatch.putUserProperty("name", "value"); | ||
|
|
| @Override | ||
| public void putUserProperty(String name, String value) { | ||
| super.putUserProperty(name, value); | ||
| for (Message message : messages) { | ||
| message.putUserProperty(name, value); | ||
| } | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10588 +/- ##
=============================================
- Coverage 48.26% 48.17% -0.10%
+ Complexity 13433 13413 -20
=============================================
Files 1378 1378
Lines 100817 100827 +10
Branches 13040 13043 +3
=============================================
- Hits 48660 48570 -90
- Misses 46211 46294 +83
- Partials 5946 5963 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
MessageBatch encodes the inner message list, so user properties set only on the MessageBatch wrapper are not visible after decoding. Propagating batch-level user properties to each inner message makes MessageBatch.putUserProperty behave consistently with the encoded message payload consumed downstream. Constraint: Related to apache#9195 Rejected: Copy wrapper properties only during generateFromList | callers can set MessageBatch properties after construction Confidence: medium Scope-risk: narrow Directive: Keep MessageBatch wrapper properties and inner message properties aligned for user-facing setters that affect encoded payload semantics. Tested: mvn -pl common -Dtest=MessageEncodeDecodeTest,MessageBatchTest,MessageTest test Not-tested: Full repository test suite
Aias00
force-pushed
the
fix/message-batch-user-property
branch
from
967de48 to
e0c0fff
Compare
|
Updated after review: removed the batch-level |
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Overrides MessageBatch.putUserProperty() to propagate user properties to all inner messages, ensuring they survive encode/decode. Fixes #9195.
Findings
-
[Info]
MessageBatch.java:42-50— The null guard onmessagesis good defensive coding. Sinceencode()would NPE on null messages anyway, this is consistent. -
[Info]
MessageBatch.java:42-50— Consider also overriding other property mutation methods onMessage(e.g.,removeUserProperty,setProperties) for consistency. If a caller removes a property from the batch wrapper, inner messages would still carry it, creating an asymmetric state. This is not a blocker for this PR but worth a follow-up. -
[Info]
MessageBatch.java:46— The for-each loop iteratesmessageswithout synchronization. Sincemessagesis afinalreference to a list created internally bygenerateFromList, this is safe in normal usage. If a caller constructsMessageBatchdirectly with a mutable list and modifies it concurrently,ConcurrentModificationExceptionis possible — butMessageBatchis not documented as thread-safe, so this is acceptable.
Suggestions
The test coverage is solid:
testPutUserPropertyWithNullMessages— null guard verification ✓testMessageBatchEncodePreservesInnerMessageUserProperty— existing behavior preserved ✓testMessageBatchUserPropertyPropagatesToInnerMessages— new propagation behavior ✓
One minor suggestion: consider adding a test that verifies getUserProperty on the batch wrapper returns the same value after propagation, to document the dual-write behavior explicitly.
Verdict
Clean fix with good test coverage. The approach is straightforward and the null guard handles the edge case well.
Automated review by github-manager-bot
The review asked to make the dual-write behavior explicit in tests. The existing propagation test already exercises batch-level property propagation into encoded inner messages, so the narrowest fix is to assert the wrapper still exposes the same property after putUserProperty. Constraint: PR scope is limited to MessageBatch user-property propagation for issue apache#9195 Rejected: Override removeUserProperty and setProperties | review marked this as non-blocking and it would expand the behavioral surface beyond the bug fix Confidence: high Scope-risk: narrow Directive: Keep this PR focused on putUserProperty propagation unless maintainers explicitly ask for broader Message property mutation symmetry Tested: mvn -pl common -Dtest=MessageEncodeDecodeTest,MessageBatchTest,MessageTest test Not-tested: Full repository test suite
|
Addressed the review suggestion by extending the propagation regression test to assert that the MessageBatch wrapper still returns the user property after putUserProperty, documenting the dual-write behavior explicitly.\n\nVerified with:\n |
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot (follow-up)
Update since last review
New commit c181019 adds assertEquals("value", messageBatch.getUserProperty("name")) in testMessageBatchUserPropertyPropagatesToInnerMessages — this addresses the previous review suggestion to explicitly verify the dual-write behavior on the batch wrapper.
Verdict
The suggestion has been addressed. The PR now has complete test coverage for the propagation behavior:
- ✅ Null guard (
testPutUserPropertyWithNullMessages) - ✅ Inner message property preserved during encode/decode
- ✅ Property propagation to inner messages
- ✅ Wrapper
getUserPropertyreturns correct value after propagation
No further concerns. LGTM.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Fixes #9195
Brief Description
MessageBatchencodes the inner message list viaMessageDecoder.encodeMessages(messages). If a caller sets a user property on theMessageBatchwrapper itself, that wrapper property is not part of the encoded inner messages and therefore is not visible after decoding or consumption.This PR makes
MessageBatch.putUserProperty(...)propagate the user property to every inner message while still preserving the property on the wrapper. This keeps batch-level user property setters aligned with the payload that is actually encoded and consumed.The tests also cover the existing behavior where user properties already set on inner messages are preserved during batch encode/decode.
How Did You Test This Change?
mvn -pl common -Dtest=MessageEncodeDecodeTest,MessageBatchTest,MessageTest testResult:
BUILD SUCCESS,Tests run: 17, Failures: 0, Errors: 0, Skipped: 0