Repository navigation
Conversation
Signed-off-by: zhang hongyuan <100520587+Metastarx@users.noreply.github.com>
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR adds GZIP as a fourth selectable message-body compression codec, implemented purely on java.util.zip so the compressed output is interchangeable with standard gzip tools in other languages. The change extends the CompressionType enum (value 4), the CompressorFactory, the system-flag bit allocation, and the producer compression-type plumbing.
Findings
-
[Info]
GzipCompressor.java:43-46— TheLevelGzipOutputStreaminner class reaches into the protecteddeffield inherited fromDeflaterOutputStreamto callsetLevel()after the super constructor has already created theDeflater. This is the standard workaround forGZIPOutputStreamnot exposing a level-aware constructor, and it works correctly. Worth a one-line comment noting thatdefis inherited fromDeflaterOutputStreamso future readers do not have to trace the hierarchy. -
[Info]
GzipCompressor.java:50-73— The compress path callsfinish()thenclose()inside the try, and the finally block closes both streams again. This is safe (double-close is a no-op for these streams) and correctly releases the nativeDeflateron the error path. Consistent with the existingZlibCompressorpattern. -
[Info]
GzipCompressor.java:76-110— The decompress buffer is allocated atsrc.length, which is the compressed size. For payloads that expand significantly (e.g. highly compressed binary data), this means many smallread()iterations. The existingZlibCompressoruses the same pattern, so this is consistent. If large-message decompression becomes a hot path later, a larger fixed buffer (e.g. 8 KB) could reduce syscall overhead, but this is not a blocker. -
[Info]
CompressionType.java:82-84— The comment "4 is the first value that still fits in the three bits reserved byCOMPRESSION_TYPE_COMPARATOR(0x7)" is helpful for future contributors. The 3-bit mask supports values 0-7, so values 5-7 remain available for future codecs. -
[Info]
MessageSysFlag.java:49—COMPRESSION_GZIP_TYPE = 0x4 << 8correctly occupies the next slot in the 3-bit compression field. Wire-protocol compatible: older brokers that do not recognize value 4 will hit the existingdefaultbranch ingetCompressionType()and throw, which is the expected fail-fast behavior.
Suggestions
-
Broker-side awareness: Confirm that the broker
MessageDecoderand any downstream consumer tooling (e.g.mqadmin checkMsg, export tools) can handleCOMPRESSION_GZIP_TYPEin the sysflag. If the broker decodes compression type only for logging or routing, a new value should not break anything, but it is worth verifying. -
Documentation: Consider adding GZIP to the user-facing documentation (e.g. the compression configuration guide) so users know they can set
compressType=GZIPon the producer. -
CompressionFlagTest.testCompressionFlagNotMatch: The update from0x4 << 8to0x5 << 8for the "not match" case is correct — value 4 is now valid, so 5 is the right choice for the negative test.
Overall
The implementation is clean, well-tested (round-trip at level bounds, empty body, large body, repeatability, invalid data, null input), and follows the established patterns of the existing codecs. No functional concerns.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Brief Description
RocketMQ could already compress message bodies with LZ4, ZSTD and ZLIB, but all three need a third-party library or produce framing that external tools do not read by default. This change adds GZIP as a fourth selectable codec, implemented purely with java.util.zip, so a compressed body is interchangeable with the gzip output that other languages and command line tools produce. The enum, the compressor factory, the system flag bit 4 and the producer's compression-type plumbing are all extended, and the new GzipCompressor follows the same structure as the existing codecs, including level handling through the inherited Deflater and the same stream shutdown and error propagation. Existing behaviour is untouched: ZLIB stays the default, value 0 still resolves to ZLIB, and value 4 now maps to GZIP inside the three bits the system flag already reserves.
How Did You Test This Change?
mvn test -pl client,common- passed (Tests run: 604, Failures: 0, Errors: 0, Skipped: 8)