RockteMQ-AI commented on PR #10535: URL: https://github.com/apache/rocketmq/pull/10535#issuecomment-4976328162
## Review by github-manager-bot ### Summary Re-review after rebase (commit `1994607f`, 2026-07-14). This PR implements `FastCodesHeader.encode()/decode()` for `SendMessageRequestHeader` to eliminate reflection-based header serialization on the send-message hot path. Single file change (+143/-18). ### Findings - **[Info] encode() short-key mapping** — The 14 short keys (a–n) correctly match `SendMessageRequestHeaderV2` field names. Verified mapping: a=producerGroup, b=topic, c=defaultTopic, d=defaultTopicQueueNums, e=queueId, f=sysFlag, g=bornTimestamp, h=flag, i=properties, j=reconsumeTimes, k=unitMode, l=maxReconsumeTimes, m=batch, n=brokerName. ✓ - **[Info] decode() dual-key fallback** — Each field first tries the short key, then falls back to the long key via `getAndCheckNotNull()`. This ensures backward compatibility with both V1 (long-key) and V2 (short-key) wire formats. ✓ - **[Info] parseRequestHeader() simplification** — The old switch-case with V2→V1 conversion is replaced by a single `decodeCommandCustomHeader(SendMessageRequestHeader.class)` call. Since `FastCodesHeader.decode()` handles both short and long keys, this correctly replaces both the V1 direct-decode and V2-then-convert paths. The simplification is sound. - **[Info] bornTimestamp type change** — Field changed from `Long` (boxed) to `long` (primitive), with a null-safe overload `setBornTimestamp(Long)` that defaults to `0L`. This prevents NPE from unboxing and is consistent with the `@CFNotNull` annotation. - **[Warning] @CFNotNull validation in decode()** — For `@CFNotNull` fields (producerGroup, topic, bornTimestamp, flag), `getAndCheckNotNull()` will throw `RemotingCommandException` if the key is absent. However, for nullable fields like `defaultTopic`, `queueId`, etc., the same `getAndCheckNotNull()` is used. Verify that `getAndCheckNotNull()` correctly returns `null` (rather than throwing) for fields without `@CFNotNull` — otherwise nullable fields would incorrectly throw when absent. - **[Info] Missing wire-compatibility test** — Previous review suggested adding a unit test that encodes with V2 format and decodes with the new `FastCodesHeader.decode()`. This test is not present in the current diff. Consider adding one to prevent regression. ### Suggestions 1. Verify `getAndCheckNotNull()` behavior for nullable fields (non-`@CFNotNull`). 2. Consider adding a V2→V1 decode compatibility test. 3. The `bornTimestamp` field type change from `Long` to `long` is a binary-incompatible change for any external code calling `getBornTimestamp()`. Confirm this is acceptable for the release. --- *Automated review by github-manager-bot* -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
