feat(java): support GZIP message body compression on producer side - #1304
feat(java): support GZIP message body compression on producer side#1304itxaiohanglover wants to merge 2 commits into
Conversation
…pache#1288) Introduce ProducerBuilder#setCompressBodyThresholdBytes to enable opt-in GZIP compression for message bodies that reach the threshold. Compression is disabled by default, and the consumer side already decompresses transparently according to the body encoding.
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Adds opt-in producer-side GZIP compression for message bodies in the Java client. When setCompressBodyThresholdBytes(int) is configured, messages whose body size reaches the threshold are compressed with GZIP before sending. The consumer already decompresses transparently via the Encoding magic-code, so no consumer-side changes are needed. Clean, backward-compatible design (disabled by default via Integer.MAX_VALUE).
Findings
-
[Warning]
ProducerBuilderImpl.java:104— Encapsulation violation.producer.publishingSettings.compressBodyThresholdBytes = compressBodyThresholdBytesdirectly accesses a field on the internalPublishingSettingsobject from outside the class. This breaks the pattern used by other settings (which are set via constructor orsync()). Consider either:- Adding a constructor parameter to
PublishingSettingsfor the compression threshold, or - Adding a package-private setter method on
PublishingSettingsinstead of direct field access.
This would also make the field easier to test in isolation and more resilient to future refactoring.
- Adding a constructor parameter to
-
[Info]
PublishingMessageImpl.java:60-68— Compression always wins over original. Whenbody.length >= threshold, the compressed bytes are always used — even if GZIP makes the body larger (e.g., already-compressed or encrypted payloads). This is a reasonable simplification, but you might want to add a fallback:if (compressed.length < body.length) { transportBody = compressed; encoding = Encoding.GZIP; } else { transportBody = body; encoding = Encoding.IDENTITY; }
This avoids wasting CPU on compression that increases size. If you intentionally skip this for simplicity, a brief comment in the code explaining the tradeoff would help future readers.
-
[Info]
PublishingMessageImpl.java:70-73— Max body size check uses compressed size. The checktransportBody.length > publishingSettings.getMaxBodySizeBytes()now validates the compressed body. This matches the documented intent onPublishingSettings#maxBodySizeBytes("it would be compressed for convenience of transport"), which is good. Just worth noting that a message whose original body is under the limit could still be rejected if compression inflates it pastmaxBodySizeBytes. -
[Info]
PublishingSettings.java—sync()does not touchcompressBodyThresholdBytes. This is correct (it's a client-only setting, not pushed by the server), but worth adding a brief inline comment like// Client-only: not synced from serverto prevent future confusion.
Test Coverage
Good coverage of the happy path. Consider adding:
- A test where GZIP produces a larger body (e.g., random bytes) — verify the behavior is deterministic and documented.
- A test where the compressed body exceeds
maxBodySizeBytes— verify the message is rejected. - A boundary test:
body.length == threshold(should compress per the>=check).
Cross-repo Note
The protocol Encoding enum (IDENTITY / GZIP) is defined in apache/rocketmq-apis. This PR correctly uses the existing enum values. No broker-side changes are needed since decompression happens on the consumer side. When zstd support is added later, it will require a protocol change in apache/rocketmq-apis first.
Automated review by github-manager-bot
…back, more tests - Tighten PublishingSettings#setCompressBodyThresholdBytes to package-private and document it as a client-only setting (never synced from the server) - Fall back to the identity encoding when GZIP does not reduce the body size (already-compressed or encrypted payloads), avoiding transport inflation - Add tests: exact-threshold boundary, incompressible-body fallback, oversized incompressible body rejection, compressed-body-fits acceptance
|
Thanks for the thorough review — all four points adopted in 621b7af:
All 19 tests pass locally ( |
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot (Follow-up)
New Commit: feat(java): address review — package-private setter, compression fallback, more tests
All previous review findings have been addressed:
-
Encapsulation ✅ —
setCompressBodyThresholdBytesis now package-private. A dedicatedPublishingSettingsTestHelperin the same package provides test-only access — clean pattern. -
Compression fallback ✅ —
PublishingMessageImplnow compares compressed vs. original size and falls back toIDENTITYencoding when GZIP inflates the body. This avoids wasting bandwidth on already-compressed or encrypted payloads. -
Client-only comment ✅ — Added inline comment on
setCompressBodyThresholdBytesclarifying it is never synced from the server. -
Test coverage ✅ — Four new tests:
testBodyCompressedAtExactThreshold— boundary test at exact threshold (good edge case)testBodyNotCompressedWhenGzipInflates— verifies fallback with incompressible random bytestestIncompressibleBodyExceedingMaxSizeRejected— large incompressible body correctly rejectedtestCompressibleBodyOverMaxSizeAcceptedWhenCompressedFits— large compressible body accepted when compressed size fits
Verdict
LGTM. The follow-up commit is well-structured, each change maps directly to a review finding, and the new tests cover the key edge cases. Ready for committer review.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR adds opt-in producer-side GZIP message body compression for the Java client. When a message body exceeds the configured threshold, it is compressed before sending. Compression is disabled by default (Integer.MAX_VALUE), ensuring backward compatibility.
Findings
-
[Info]
ProducerBuilder.java:85— The new APIsetCompressBodyThresholdBytes(int)is clean and follows the existing builder pattern. Javadoc is clear. -
[Info]
ProducerBuilderImpl.java:89— ValidationcompressBodyThresholdBytes > 0is correct. Consider also documenting the behavior when threshold is very small (e.g., 1 byte) — every message will be compressed, which may hurt CPU for negligible size reduction. -
[Warning]
PublishingMessageImpl.java:60-70— The compression happens in the constructor, which is called per-message on the producer hot path. GZIP compression for large messages can be CPU-intensive. Consider:- Adding a note in the Javadoc about the CPU/memory trade-off.
- Potentially using a shared
GZIPOutputStreamor buffered approach to reduce per-message allocation overhead. - Documenting recommended threshold values (e.g., 1KB, 4KB) in the API doc.
-
[Info]
PublishingMessageImpl.java:49— TheBODY_ENCODINGmessage property is a good approach for transparent consumer decompression. Ensure this property key is documented in the protocol spec or client compatibility matrix so other language clients can implement the same convention. -
[Info]
PublishingSettings.java:42— Thread safety with@GuardedBy("this")is consistent with the existing pattern. Good. -
[Info] Tests — Good coverage: builder validation, settings behavior, message compression/decompression roundtrip. The test helper
PublishingSettingsTestHelperis a nice touch for keeping tests clean.
Suggestions
- Consider adding a compression level parameter (1-9) in a future iteration, allowing users to tune the CPU vs. compression ratio trade-off.
- The
Encodingenum is package-private inmessagepackage. If other modules need to check encoding (e.g., interceptor chains), consider making it public or providing an accessor.
Cross-repo Note
The BODY_ENCODING property convention should be coordinated with the Proxy module in apache/rocketmq to ensure the Proxy does not strip or alter this message property during forwarding.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Fixes #1288
Brief Description
This PR adds opt-in producer-side message body compression for the Java client:
ProducerBuilder#setCompressBodyThresholdBytes(int): once set, a message whose body size reaches the threshold is compressed with GZIP before sending. Compression is disabled by default (Integer.MAX_VALUE), so existing behavior is unchanged.PublishingMessageImplnow carries the transport body and itsEncoding(IDENTITY/GZIP), and setsSystemProperties#bodyEncodingaccordingly, instead of always hardcodingIDENTITY.PublishingSettings#maxBodySizeBytes("it would be compressed for convenience of transport"), which was previously not implemented.No change is required on the consuming side:
MessageViewImplalready decompresses the body transparently according to the body encoding (magic-code based, GZIP supported).Regarding zstd (mentioned in #1288): the protocol-level
apache.rocketmq.v2.Encodingenum currently only definesIDENTITYandGZIP, so zstd support requires a protocol change in rocketmq-apis first. This PR takes GZIP as the first step; zstd can be a follow-up once the protocol supports it. (The compression utilities for ZSTD/ZLIB/LZ4 already exist inUtilitiesfor the consuming side.)How Did You Test This Change?
PublishingMessageImplTest:bodyEncodingisGZIP, andUtilities#decompressBytesround-trips to the original body;IDENTITY.ProducerBuilderImplTest(negative threshold rejected).ProducerImplTest/TransactionImplTestMockito errors on my machine are a pre-existing local JDK 21 incompatibility, reproduced on a clean master checkout as well.)