[Enhancement] Serialize POP checkpoint/ack messages without the intermediate JSON string
- Dominant language
- Java
- Stars
- 22.6k
- Forks
- 12k
- Avg merge
- 3d 1h
- Merged PRs (30d)
- 27
Description
### Before Creating the Enhancement Request
- [x] I have confirmed that this should be classified as an enhancement rather than a bug/feature.
### Summary
Serialize POP checkpoint/ack messages with `JSON.toJSONBytes` and deserialize them with `JSON.parseObject(byte[], ...)` instead of going through an intermediate JSON `String`.
### Motivation
Every POP checkpoint and ack that reaches the revive topic is serialized as `JSON.toJSONString(x).getBytes(UTF_8)` — a full `String` (with its internal array) plus a second `byte[]` copy per record — in five call sites: `PopMessageProcessor#buildCkMsg`, `PopBufferMergeService#putAckToStore` / `putBatchAckToStore`, `AckMessageProcessor`, and `ChangeInvisibleTimeProcessor` (ack + re-put CK). On the read side, `PopReviveService#scanReviveQueue` first materializes `new String(body)` and then parses it, for CK, ack, and batch-ack records alike.
The newer popkv implementation already does this directly: `PopConsumerRecord` uses `JSON.toJSONBytes(this)` / `JSON.parseObject(body, ...)`.
### Solution
- Replace the six encode sites with `JSON.toJSONBytes(x)`.
- Parse 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` writes the same UTF-8 bytes as `toJSONString().getBytes(UTF_8)`. New tests assert byte-for-byte equality (including non-ASCII field values) and byte-array round-trips for `PopCheckPoint`, `AckMsg`, and `BatchAckMsg`.
### Verification
- New equivalence/round-trip tests: `PopCheckPointTest`, 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, 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, 3 interleaved trials, broker jar swapped per arm): broker young GC per million consumed msgs 2.67/2.68/2.66 (base) vs 2.66/2.63/2.79 (patch) — parity, no regression. The saving itself (one string + one copy per CK/ack) is below GC-count resolution at this load; this is a cleanup-level optimization consistent with the popkv precedent.
Contributor guide
Research direction
Start with the named POP call sites: PopMessageProcessor, PopBufferMergeService, AckMessageProcessor, ChangeInvisibleTimeProcessor, and PopReviveService. Read the existing PopConsumerRecord implementation as the precedent, then run PopCheckPointTest, AckMsgTest, BatchAckMsgTest, and the touched processor tests. Done means byte-for-byte compatibility, successful byte-array round-trips, and no regression in the listed tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend, distributed-systems
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 25/100