RockteMQ-AI commented on issue #11425:
URL: https://github.com/apache/rocketmq/issues/11425#issuecomment-6149876912
**Automated Fix Proposal (v1)**
Code base: `a7edeecfb662` (develop)
# Fix Specification — Issue #11425: `compressedRegister=true` breaks broker
registration (header/body compression flag mismatch)
- **Issue**: [Bug] compressedRegister=true makes broker registration fail:
the register body is compressed but the header says it is not
- **Base**: develop @ `a7edeecfb662`
- **Verification status**: CONFIRMED (root cause reproduced by code audit
below)
- **Severity**: High — with `compressedRegister=true`, a broker can never
register with any name server; route discovery is dead for the whole cluster
until config rollback.
---
## 1. Root Cause Analysis
### Primary defect (one line)
`broker/src/main/java/org/apache/rocketmq/broker/out/BrokerOuterAPI.java:517`
```java
requestHeader.setCompressed(false); // line 517 — hardcoded,
ignores the `compressed` parameter
...
final byte[] body = requestBody.encode(compressed); // line 525 — honors
the `compressed` parameter
```
When `compressed == true`, line 525 produces a zlib **deflate stream**
(`RegisterBrokerBody.encode(true)`,
`remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/RegisterBrokerBody.java:51-113`),
while line 517 tells the name server the body is **plain JSON**.
### How the `compressed` parameter flows
- `BrokerConfig.compressedRegister` (default `false`,
`common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java:180`) is
passed as the `compressed` argument via `BrokerController.doRegisterBrokerAll`
→ `brokerOuterAPI.registerBrokerAll(...)`
(`broker/src/main/java/org/apache/rocketmq/broker/BrokerController.java:2158-2171`,
specifically line 2169: `this.brokerConfig.isCompressedRegister()`).
- All registration triggers flow through this single path: initial startup
registration, the periodic re-registration task
(`BrokerController.java:1983,1998`), `forceRegister`, dLeger role change
(`DLedgerRoleChangeHandler.java:149,172`), controller-mode
(`ReplicasManager.java:326`), container mode
(`InnerBrokerController.java:81,96`), and admin-triggered registration
(`AdminBrokerProcessor.java:1153`). So the bug hits every registration attempt,
not just startup.
### Failure mode on the name server
<details>
<summary>Full Specification</summary>
# Fix Specification — Issue #11425: `compressedRegister=true` breaks broker
registration (header/body compression flag mismatch)
- **Issue**: [Bug] compressedRegister=true makes broker registration fail:
the register body is compressed but the header says it is not
- **Base**: develop @ `a7edeecfb662`
- **Verification status**: CONFIRMED (root cause reproduced by code audit
below)
- **Severity**: High — with `compressedRegister=true`, a broker can never
register with any name server; route discovery is dead for the whole cluster
until config rollback.
---
## 1. Root Cause Analysis
### Primary defect (one line)
`broker/src/main/java/org/apache/rocketmq/broker/out/BrokerOuterAPI.java:517`
```java
requestHeader.setCompressed(false); // line 517 — hardcoded,
ignores the `compressed` parameter
...
final byte[] body = requestBody.encode(compressed); // line 525 — honors
the `compressed` parameter
```
When `compressed == true`, line 525 produces a zlib **deflate stream**
(`RegisterBrokerBody.encode(true)`,
`remoting/src/main/java/org/apache/rocketmq/remoting/protocol/body/RegisterBrokerBody.java:51-113`),
while line 517 tells the name server the body is **plain JSON**.
### How the `compressed` parameter flows
- `BrokerConfig.compressedRegister` (default `false`,
`common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java:180`) is
passed as the `compressed` argument via `BrokerController.doRegisterBrokerAll`
→ `brokerOuterAPI.registerBrokerAll(...)`
(`broker/src/main/java/org/apache/rocketmq/broker/BrokerController.java:2158-2171`,
specifically line 2169: `this.brokerConfig.isCompressedRegister()`).
- All registration triggers flow through this single path: initial startup
registration, the periodic re-registration task
(`BrokerController.java:1983,1998`), `forceRegister`, dLeger role change
(`DLedgerRoleChangeHandler.java:149,172`), controller-mode
(`ReplicasManager.java:326`), container mode
(`InnerBrokerController.java:81,96`), and admin-triggered registration
(`AdminBrokerProcessor.java:1153`). So the bug hits every registration attempt,
not just startup.
### Failure mode on the name server
`namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/DefaultRequestProcessor.java`:
1. `registerBroker(...)` (line 218) first runs `checksum(...)` (line 225) —
**this passes**, because CRC32 is computed over the same (compressed) bytes on
both sides. The corruption is purely semantic.
2. `extractRegisterBrokerBodyFromRequest(...)` (line 291) calls
`RegisterBrokerBody.decode(request.getBody(),
requestHeader.isCompressed(), brokerVersion)` (line 298).
3. With `isCompressed() == false`, decode takes the JSON branch
(`RegisterBrokerBody.java:116-118`: `RegisterBrokerBody.decode(data,
RegisterBrokerBody.class)`) and attempts to JSON-parse the deflate stream →
exception → rethrown as `RemotingCommandException("Failed to decode
RegisterBrokerBody")` → name server returns `SYSTEM_ERROR`.
4. Broker logs `Failed to register current broker to name server` and
retries at every registration cycle — registration never succeeds.
### Why the header flag will actually reach the name server after the fix
The `compressed` field is a plain (unannotated) `boolean` on
`RegisterBrokerRequestHeader`
(`remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/namesrv/RegisterBrokerRequestHeader.java:51`).
It round-trips through extFields because:
- `RemotingCommand.makeCustomHeaderToNet()`
(`remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RemotingCommand.java:430-456`)
serializes **all** non-static fields — including unannotated ones — into
`extFields`.
- `RemotingCommand.decodeCommandCustomHeader` (same file, ~line 290-345)
reads all non-static fields back (`Boolean.parseBoolean` for booleans;
unannotated fields are treated as nullable).
So restoring `setCompressed(compressed)` makes the truthful flag reach the
NS, which then takes the symmetric `InflaterInputStream` decode path
(`RegisterBrokerBody.java:115-170`) — the mirror image of the broker's
`DeflaterOutputStream` encode path, including the `V5_0_0` topic-queue-mapping
section.
### Regression origin
Commit `cad0cfcec` changed `setCompressed(compressed)` →
`setCompressed(false)` (motivated by the javadoc note at
`BrokerOuterAPI.java:475-478`: "Considering compression brings much CPU
overhead to name server, stream API will not support compression and
compression feature is deprecated") but **kept `encode(compressed)`**. The
result is a header/body disagreement that only manifests when a user opts in
via `compressedRegister=true` — the config remained public and
functional-looking (`BrokerConfig.java:1328-1334`), so nothing alerted the
user. The local repo is a shallow clone (single commit), so the regression
commit itself could not be re-inspected; the evidence is the current
inconsistent state, which fully matches the issue description.
### Why existing tests missed it
`broker/src/test/java/org/apache/rocketmq/broker/BrokerOuterAPITest.java:197-210`
(`test_register_normal`) already passes `compressed=true`, but the mocked
`NettyRemotingClient` never decodes the body nor inspects the header — it just
returns a canned SUCCESS response. The test validates the broker-side happy
path only; the mismatch is invisible to it.
---
## 2. Fix Strategy
**Chosen fix: make the header tell the truth (restore pre-regression
behavior).**
### Step 1 — The one-line fix (required)
In
`broker/src/main/java/org/apache/rocketmq/broker/out/BrokerOuterAPI.java:517`,
change:
```java
requestHeader.setCompressed(false);
```
to:
```java
requestHeader.setCompressed(compressed);
```
This restores header/body consistency for both values of the flag:
- `compressed=false` (default): header says `false`, body is JSON —
unchanged wire behavior for the overwhelming majority of deployments.
- `compressed=true`: header says `true`, body is deflate — name server takes
the correct inflate/decode path, exactly as it did before commit `cad0cfcec`.
### Step 2 — Correct the misleading javadoc (recommended, prevents
recurrence)
The javadoc at `BrokerOuterAPI.java:475-490` ("compression feature is
deprecated") is factually wrong post-fix and is precisely the comment that
accompanied the regression. Replace:
```java
/**
* Considering compression brings much CPU overhead to name server, stream
API will not support compression and
* compression feature is deprecated.
* ...
* @param compressed default false
*/
```
with:
```java
/**
* The register body is compressed (zlib deflate) if and only if {@code
compressed} is true, and the
* request header's compressed flag is set accordingly so the name server
decodes it symmetrically.
* The stream API does not support compression; this remoting path honors
the broker's
* compressedRegister configuration, default false.
* ...
* @param compressed whether to compress the register body, default
false
*/
```
### Alternative considered and rejected
Changing line 525 to `encode(false)` instead (fully kill compression) would
also stop the crash, but it silently turns the public, documented
`compressedRegister` config into a no-op — a config that lies to users. If the
project ever wants to truly deprecate the feature, that must be done explicitly
(deprecate the config, log a warning), not via a hidden flag mismatch. The
issue's verification result also prescribes `setCompressed(compressed)`.
### What is explicitly out of scope
- Any change to `RegisterBrokerBody` encode/decode (symmetric and
long-standing).
- Any change to `DefaultRequestProcessor` (its logic is correct once the
flag is truthful).
- The pre-existing edge case where `encode(true)` returns `null` on
`IOException` (`RegisterBrokerBody.java:108-112`), which would NPE at
`UtilAll.crc32(body)`. That behavior predates the regression and is unchanged
by this fix; file a separate issue if desired.
---
## 3. Files to Modify
| # | File | Change |
|---|------|--------|
| 1 |
`broker/src/main/java/org/apache/rocketmq/broker/out/BrokerOuterAPI.java` |
**Line 517**: `setCompressed(false)` → `setCompressed(compressed)`. |
| 2 |
`broker/src/main/java/org/apache/rocketmq/broker/out/BrokerOuterAPI.java` |
**Lines 475-490** (optional, recommended): javadoc correction per Step 2. |
Test files (see §4):
| # | File | Change |
|---|------|--------|
| 3 |
`broker/src/test/java/org/apache/rocketmq/broker/BrokerOuterAPITest.java` | Add
regression test(s) asserting header/body agreement. |
| 4 |
`namesrv/src/test/java/org/apache/rocketmq/namesrv/processor/RequestProcessorTest.java`
| Add compressed-register success + mismatch-failure cases (optional but
cheap). |
No changes to `remoting` module production code. No protocol/schema changes
(the existing `RegisterBrokerRequestHeader` schema already covers
`setCompressed`/`isCompressed`).
---
## 4. Test Plan
### 4.1 Broker side — header/body agreement test (required)
Add to
`broker/src/test/java/org/apache/rocketmq/broker/BrokerOuterAPITest.java`
(reuse existing `init()` / mocked `NettyRemotingClient` harness from
`test_register_normal`):
**`test_register_compressed_header_matches_body`**:
1. Call `registerBrokerAll(..., compressed = true, ...)` with a non-empty
`TopicConfigSerializeWrapper`.
2. Capture the `RemotingCommand` passed to `invokeSync` with
`ArgumentCaptor`.
3. Important harness detail: with a mocked client the command is never
serialized, so extFields are not populated yet — the test must call
`captured.makeCustomHeaderToNet()` and then
`captured.decodeCommandCustomHeader(RegisterBrokerRequestHeader.class)`; assert
`header.isCompressed() == true` (or assert
`captured.getExtFields().get("compressed").equals("true")`).
4. Prove body/flag agreement end-to-end:
`RegisterBrokerBody.decode(captured.getBody(), true, MQVersion.Version.V5_0_0)`
must succeed and reproduce the registered topic count.
**`test_register_uncompressed_header_matches_body`** (mirror, guards the
default path):
1. Call with `compressed = false`.
2. Assert header `isCompressed() == false` and that
`RegisterBrokerBody.decode(captured.getBody(), false, version)` (JSON path)
succeeds.
These two tests fail on the unfixed code (test 1 header says `false` while
body is deflate; decoding with `true` fails) and pass after the fix.
### 4.2 Name server side — decode both flags correctly (recommended)
Extend
`namesrv/src/test/java/org/apache/rocketmq/namesrv/processor/RequestProcessorTest.java`
(it already builds REGISTER_BROKER commands manually, see
`requestBody.encode(false)` around line 623):
- **Compressed success case**: build body via `requestBody.encode(true)`,
add ext field `compressed=true` (the test constructs extFields manually, so
`request.addExtField("compressed", "true")`), assert response code `SUCCESS`
and that the broker/topic route is registered.
- **Mismatch failure case (documents the bug)**: body `encode(true)` + ext
field `compressed=false`, assert non-SUCCESS response — this locks in the
failure mode so any future header/body divergence is caught by a red test.
### 4.3 Existing tests to keep green (regression suite)
-
`remoting/src/test/java/org/apache/rocketmq/remoting/protocol/RegisterBrokerBodyTest.java`
— already covers the encode(true)/decode(true) round-trip; no change needed.
- `BrokerOuterAPITest#test_register_normal`, `test_register_timeout` — must
stay green (they pass `compressed=true` / use the default path).
- Existing `RequestProcessorTest` register/unregister cases — must stay
green.
### 4.4 Commands
```bash
mvn -pl broker -am test -Dtest=BrokerOuterAPITest
mvn -pl namesrv -am test -Dtest=RequestProcessorTest
mvn -pl remoting -am test -Dtest=RegisterBrokerBodyTest
# broader safety net:
mvn -pl broker,namesrv,remoting test
```
### 4.5 Manual verification (optional, end-to-end)
Start a namesrv + broker with `-DcompressedRegister=true` (or
`brokerConfig.compressedRegister=true` in conf), confirm `broker` logs
`Registering current broker to name server completed` and `sh mqadmin topicList
-n <namesrv>` shows system/registered topics; the namesrv log should not
contain `Failed to decode RegisterBrokerBody`. Repeat with default `false` to
confirm the unchanged default path.
---
## 5. Backward Compatibility
- **Default deployments (compressedRegister=false, the vast majority): zero
behavior change.** The header flag stays `false` and the body stays JSON —
bit-for-bit the same request as before this fix and before the regression.
- **Opt-in users (compressedRegister=true): the fix restores the
pre-`cad0cfcec` behavior.** These users are currently hard-broken (registration
always fails); after the fix they work again. The compressed decode path in the
name server (`RegisterBrokerBody.decode(body, compressed, version)`) and the
`compressed` extField have been part of the protocol for many releases, so a
fixed broker registers fine against older name servers, and older brokers are
unaffected against any NS.
- **No wire-protocol change**: no new fields, no type changes — only the
value of the existing `compressed` extField becomes truthful when compression
is enabled.
- **No API/config surface change**: `BrokerConfig.compressedRegister` keeps
its name, default, and semantics.
- **Mixed-version clusters**: new broker + old NS with compression on →
works (identical to RocketMQ pre-regression). Old broker + new NS → unaffected
(its header/body were already consistent per its own version).
- **Not a breaking change in any sense**; it is a strict regression repair.
## 6. Risk Assessment
| # | Risk | Likelihood | Impact | Mitigation |
|---|------|-----------|--------|------------|
| 1 | **Design-intent conflict**: if `cad0cfcec` truly intended to disable
compression globally (ignoring the config), this fix "reverts" that intent and
re-introduces NS-side inflate CPU overhead for opt-in users. | Low | Low | The
config is public, documented, and default-off; honoring it is the correct
contract. CPU overhead only applies to clusters that explicitly asked for it.
If deprecation is genuinely desired, do it explicitly (deprecate config + warn
log) in a separate change. |
| 2 | **PowerMock test fragility**: the new broker-side test depends on
calling `makeCustomHeaderToNet()` manually because the mocked client never
serializes; forgetting this makes assertions fail confusingly. | Medium | Low
(test-only) | Documented in §4.1 step 3; assert on extFields directly as a
fallback. |
| 3 | **Very old name servers** (pre-3.0.11 decode path,
`DefaultRequestProcessor.java:235-242`) combined with compressed bodies from a
modern broker. | Very low | Medium | That path is gated on broker *version*,
not the compressed flag, and such version combinations are already unsupported
for other reasons. Not introduced by this fix. |
| 4 | **Pre-existing `encode(true)` → `null` on IOException**
(`RegisterBrokerBody.java:108-112`) which would NPE at `UtilAll.crc32`
(BrokerOuterAPI.java:526). | Very low | Medium | Pre-existing since before the
regression; unchanged by this fix. Out of scope; recommend a follow-up issue to
fail-fast instead of returning null. |
| 5 | **Compression content asymmetry**: broker compresses DataVersion +
topic configs + filter list + topic-queue-mappings, but NS only parses
topic-queue-mappings for broker ≥ V5_0_0 (`RegisterBrokerBody.java:152`). A
<5.0 broker's mapping section would be silently skipped. | Very low | Low |
Long-standing protocol logic, untouched by this fix. |
| 6 | **Rollback risk**: essentially none — the fix is one line and
trivially revertible. | — | — | — |
**Overall risk: low.** The change is a single-line truth restoration on a
default-off path, with the golden path (uncompressed) provably untouched.
</details>
**Next Steps:**
- Reply `/approve` to proceed with PR generation
- Reply `/revise <feedback>` to request changes
- Reply `/reject` to close this proposal
*This proposal expires in 72 hours.*
---
*Automated 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]
