[ISSUE #10980] Encode and decode POP checkpoint and ack messages without the intermediate JSON string

[wang-jiahua]

Which Issue(s) This PR Fixes

Fixes #10980

Brief Description

Every POP checkpoint/ack written to the revive topic was serialized as JSON.toJSONString(x).getBytes(UTF_8) — a full intermediate String plus a second byte[] copy per record — and PopReviveService#scanReviveQueue materialized new String(body) before parsing each CK/ack/batch-ack record. This PR switches the six encode sites (PopMessageProcessor#buildCkMsg, PopBufferMergeService#putAckToStore/putBatchAckToStore, AckMessageProcessor, ChangeInvisibleTimeProcessor ack + re-put CK) to JSON.toJSONBytes(x) and parses revive records straight from messageExt.getBody(); the raw string is now built only inside the enablePopLog branch that logs it.

The stored bytes are unchanged — fastjson2's toJSONBytes produces the same UTF-8 bytes as toJSONString().getBytes(UTF_8) — and the newer popkv implementation (PopConsumerRecord) already uses exactly this pattern.

How Did You Test This Change?

• New byte-equivalence and byte-array round-trip tests (including non-ASCII field values): PopCheckPointTest (new), plus new cases in AckMsgTest / BatchAckMsgTest — 5/5 pass.
• Single-class clean runs of the touched broker tests: AckMessageProcessorTest 8/8, ChangeInvisibleTimeProcessorTest 9/9, PopMessageProcessorTest 8/8, PopBufferMergeServiceTest 4/4. PopReviveServiceTest matches the develop baseline exactly (one pre-existing failure — a V1/V2 retry-topic naming assertion — identical with and without this change).
• 4-node cluster A/B in POP mode (mqadmin setConsumeMode -m POP, producer 64 threads + consumer 20 threads, consume TPS steady at ~150k, pop path confirmed active via pop.log, per arm clean store + broker restart + page cache drop, 3 interleaved trials, broker jar swapped per arm): broker young GC per million consumed messages 2.67/2.68/2.66 (base) vs 2.66/2.63/2.79 (patch) — parity, zero failures, no regression. The saving itself (one string + one array copy per CK/ack) is below GC-count resolution at this load, so this is a cleanup-level optimization aligned with the popkv precedent.

[Copilot]

Pull request overview

Warning

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.

Optimizes POP revive-topic checkpoint/ack serialization and parsing by eliminating intermediate String creation, reducing per-record allocations while preserving the stored UTF-8 JSON bytes.

Changes:

• Switch POP CK/ACK/BATCH_ACK encoding from toJSONString(...).getBytes(UTF_8) to JSON.toJSONBytes(...).
• Parse revive-topic records directly from messageExt.getBody() instead of new String(body) + parse.
• Add unit tests to validate byte equivalence and byte[] round-trip decoding (including non-ASCII content).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
store/src/test/java/org/apache/rocketmq/store/pop/PopCheckPointTest.java	Adds byte-equivalence + byte[] round-trip test for PopCheckPoint.
store/src/test/java/org/apache/rocketmq/store/pop/BatchAckMsgTest.java	Adds byte-equivalence + byte[] round-trip test for BatchAckMsg.
store/src/test/java/org/apache/rocketmq/store/pop/AckMsgTest.java	Adds byte-equivalence + byte[] round-trip test for AckMsg.
broker/src/main/java/org/apache/rocketmq/broker/processor/PopReviveService.java	Parses CK/ACK/BATCH_ACK directly from message body bytes; only builds raw string when logging is enabled.
broker/src/main/java/org/apache/rocketmq/broker/processor/PopMessageProcessor.java	Encodes CK messages via JSON.toJSONBytes.
broker/src/main/java/org/apache/rocketmq/broker/processor/PopBufferMergeService.java	Encodes ACK/BATCH_ACK via JSON.toJSONBytes.
broker/src/main/java/org/apache/rocketmq/broker/processor/ChangeInvisibleTimeProcessor.java	Encodes ACK and (re-)put CK via JSON.toJSONBytes.
broker/src/main/java/org/apache/rocketmq/broker/processor/AckMessageProcessor.java	Encodes ACK/BATCH_ACK via JSON.toJSONBytes.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[RockteMQ-AI]

LGTM — clean, focused performance optimization that eliminates unnecessary intermediate String allocations on the POP checkpoint/ack encode and decode paths.

Summary

Replaces JSON.toJSONString(x).getBytes(UTF_8) with JSON.toJSONBytes(x) at all 6 encode sites, and new String(body) + JSON.parseObject(raw, ...) with direct JSON.parseObject(body, ...) at all 3 decode sites in PopReviveService. Since fastjson2 uses UTF-8 internally for both toJSONBytes and parseObject(byte[]), the serialized form is byte-identical to the previous path — no behavioral change.

The roundtrip equivalence tests (JsonBytesRoundtripTest) covering all 6 message types (including Chinese characters and $ in keys) provide good confidence that the optimization is safe.

Unused StandardCharsets imports are properly cleaned up.

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 53.33333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.52%. Comparing base (e348efa) to head (f2696b9).

Files with missing lines	Patch %	Lines
...he/rocketmq/broker/processor/PopReviveService.java	33.33%	3 Missing and 3 partials ⚠️
...cketmq/broker/processor/PopBufferMergeService.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10981      +/-   ##
=============================================
- Coverage      48.58%   48.52%   -0.06%     
+ Complexity     13676    13674       -2     
=============================================
  Files           1381     1381              
  Lines         101475   101472       -3     
  Branches       13190    13190              
=============================================
- Hits           49299    49237      -62     
- Misses         46174    46214      +40     
- Partials        6002     6021      +19     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[RockteMQ-AI]

Summary

This PR optimizes POP checkpoint and ack message encoding/decoding by eliminating intermediate String allocations. The changes switch from JSON.toJSONString(x).getBytes(UTF_8) to JSON.toJSONBytes(x) at encode sites, and parse directly from byte arrays at decode sites in PopReviveService. The stored bytes remain unchanged (fastjson2 produces identical UTF-8 output), ensuring backward compatibility.

Verdict: LGTM — Clean optimization with good test coverage.

Review Notes

• Correctness: The byte-equivalence is correctly verified in tests, including non-ASCII character handling. The decode path changes are safe since JSON.parseObject(byte[], Class) is equivalent to the previous new String(body) + parseObject(String, Class) pattern.
• Performance: Eliminates one intermediate String and one byte[] copy per CK/ack record. While the per-record saving is small, it aligns with the existing PopConsumerRecord pattern and reduces GC pressure on the hot POP consume path.
• Logging improvement: The additional POP_LOGGER.isInfoEnabled() guard prevents unnecessary String construction when logging is enabled but info level is not — nice defensive optimization.
• Tests: Good coverage with byte-equivalence tests (testToJsonBytesMatchesJsonStringBytes) and round-trip tests. The new PopCheckPointTest class fills a gap in test coverage.

---

Automated review by github-manager-bot