apache / apache/rocketmq

[Enhancement] Serialize POP checkpoint/ack messages without the intermediate JSON string

Closed
#10,980 2 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.