You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@rocketmq.apache.org by "wang-jiahua (via GitHub)" <gi...@apache.org> on 2026/06/21 01:49:58 UTC

[PR] [ISSUE #10534] Implement FastCodesHeader encode/decode for PullMessageRequestHeader/ResponseHeader [rocketmq]

wang-jiahua opened a new pull request, #10536:
URL: https://github.com/apache/rocketmq/pull/10536

   <!-- Please make sure the target branch is right. In most case, the target branch should be `develop`. -->
   
   ### Which Issue(s) This PR Fixes
   
   Fixes #10534
   
   ### Brief Description
   
   Implement `FastCodesHeader.encode()/decode()` for `PullMessageRequestHeader` and `PullMessageResponseHeader` to eliminate reflection-based header serialization on the pull message hot path.
   
   1. **`PullMessageRequestHeader`** — Fix `decode()` signature from `Map<String, String>` to `HashMap<String, String>`, optimize `encode()` with `writeLong`/`writeInt` for numeric fields.
   2. **`PullMessageResponseHeader`** — Fix `decode()` signature, optimize `encode()` for 4 response fields.
   
   ### How Did You Test This Change?
   
   1. **Compilation**: `remoting` module compiles cleanly on JDK 21.
   2. **Commercial compatibility**: No commercial classes extend `PullMessageRequestHeader` or `PullMessageResponseHeader`.
   3. **Microbenchmark** (R1 encode/decode bench): `writeIfNotNull` + `writeDecimalInt` is 25% faster than `value.toString()` + `writeStr` for encoding; `readStr` with single-byte cache is 56% faster for decoding.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [ISSUE #10534] Implement FastCodesHeader encode/decode for PullMessageRequestHeader/ResponseHeader [rocketmq]

Posted by "RongtongJin (via GitHub)" <gi...@apache.org>.

RongtongJin commented on code in PR #10536:
URL: https://github.com/apache/rocketmq/pull/10536#discussion_r3449490317


##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/PullMessageRequestHeader.java:
##########
@@ -85,13 +85,13 @@ public void encode(ByteBuf out) {
         writeIfNotNull(out, "topic", topic);
         writeIfNotNull(out, "liteTopic", liteTopic);
         writeIfNotNull(out, "queueId", queueId);
-        writeIfNotNull(out, "queueOffset", queueOffset);
+        writeLong(out, "queueOffset", queueOffset);
         writeIfNotNull(out, "maxMsgNums", maxMsgNums);
         writeIfNotNull(out, "sysFlag", sysFlag);
-        writeIfNotNull(out, "commitOffset", commitOffset);
-        writeIfNotNull(out, "suspendTimeoutMillis", suspendTimeoutMillis);
+        writeLong(out, "commitOffset", commitOffset);
+        writeLong(out, "suspendTimeoutMillis", suspendTimeoutMillis);
         writeIfNotNull(out, "subscription", subscription);
-        writeIfNotNull(out, "subVersion", subVersion);
+        writeLong(out, "subVersion", subVersion);

Review Comment:
   This changes the missing-field behavior for `subVersion`: the old `Long` + `writeIfNotNull` path omitted it when unset, but the new primitive path always serializes `0`. There are proxy paths that set `subscription`/`expressionType` without setting `subVersion` (`PullMessageActivity` when it injects a subscription, and `ConsumerProcessor#pullMessage` when building the header). The broker uses `requestHeader.getSubVersion()` as the client filter version for non-tag filters, so defaulting to `0` can hide a missing required field and register/compare stale SQL filter data. Please either keep `subVersion` nullable/omitted here, or set it from `SubscriptionData#getSubVersion()` in those proxy paths and add a regression test.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [ISSUE #10534] Implement FastCodesHeader encode/decode for PullMessageRequestHeader/ResponseHeader [rocketmq]

Posted by "RongtongJin (via GitHub)" <gi...@apache.org>.

RongtongJin commented on code in PR #10536:
URL: https://github.com/apache/rocketmq/pull/10536#discussion_r3449490643


##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/PullMessageResponseHeader.java:
##########
@@ -52,10 +52,10 @@ public void checkFields() throws RemotingCommandException {
 
     @Override
     public void encode(ByteBuf out) {
-        writeIfNotNull(out, "suggestWhichBrokerId", suggestWhichBrokerId);
-        writeIfNotNull(out, "nextBeginOffset", nextBeginOffset);
-        writeIfNotNull(out, "minOffset", minOffset);
-        writeIfNotNull(out, "maxOffset", maxOffset);
+        writeLong(out, "suggestWhichBrokerId", suggestWhichBrokerId);

Review Comment:
   These response fields also changed from "omit when unset" to "encode explicit 0". `PullMessageProcessor` creates this response header before validation, and several early error returns only set `forbiddenType`, code, and remark, so those responses will now include `suggestWhichBrokerId=0`, `nextBeginOffset=0`, `minOffset=0`, and `maxOffset=0` even though they were never populated. That can hide initialization bugs and changes the wire shape for error responses. Please keep the boxed/omitted semantics for response fields, or add tests proving every encoded response path intentionally carries those zero values.



##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/PullMessageRequestHeader.java:
##########
@@ -47,19 +47,19 @@ public class PullMessageRequestHeader extends TopicQueueRequestHeader implements
     @CFNotNull
     private Integer queueId;
     @CFNotNull
-    private Long queueOffset;
+    private long queueOffset;

Review Comment:
   This is a protocol header API/schema surface change. The schema baselines still declare these fields as `java.lang.Long` with boxed setters in `test/src/test/resources/schema/protocol/common.protocol.header.PullMessageRequestHeader.schema` and `PullMessageResponseHeader.schema`, while this PR changes the storage type to primitive and adds primitive setter overloads. Please update the schema expectations, or explicitly document why this protocol/API baseline should remain unchanged.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org