iloveeyjafjalla opened a new pull request, #11430:
URL: https://github.com/apache/rocketmq/pull/11430
### Which Issue(s) This PR Fixes
- Fixes #11429
### Brief Description
The RocksDB variant of the subscription group manager persisted
`JSON.toJSONString(this.getForbiddenTable().get(group))` after every forbidden
update. Once the group has no forbidden bits left, the table holds no entry for
it, so the value written to the `forbidden` column family was the literal
**`null`** (`RocksDBSubscriptionGroupManager.updateForbidden` / `setForbidden`
/ `clearForbidden`, previously `:276-306`).
That value cannot be read back: `decodeForbidden` does
`JSON.parseObject(...)` and immediately calls `jsonObject.entrySet()`, so a
`null` body throws, `loadForbidden` returns `false`, `load()` returns `false`,
`BrokerController.initializeMetadata()` returns `false` and `BrokerStartup`
exits with `System.exit(-3)` — the broker cannot start from its own persisted
configuration until that key is repaired.
This change:
1. routes the three overrides through one `persistForbidden(group)` helper
that **deletes** the column-family key when the group has no forbidden bits
left (instead of writing `JSON.toJSONString(null)`), and keeps writing the
topic → bits map otherwise;
2. makes `decodeForbidden` ignore an empty body (`log.warn` + return) so
brokers that already stored a `null` value can start again after upgrading.
The related cross-topic loss when a group has several forbidden topics is
reported separately in #11093 (base class `updateForbiddenValue`); the two
fixes are independent and compose — with either one applied, clearing the last
bit of a group no longer writes an unreadable value.
### How Did You Test This Change?
New tests in `RocksDBSubscriptionGroupManagerTest`, which already injects a
mocked `RocksDBConfigManager` (no real RocksDB needed):
- `testClearForbiddenDoesNotPersistNullValue`: forbids then clears one topic
and asserts that `put("forbidden", group, "null")` is never called and that the
persisted key is deleted;
- `testDecodeForbiddenIgnoresNullBody`: feeds `decodeForbidden` a `"null"`
body and asserts it neither throws nor creates an entry.
Results on Temurin JDK 8 (the JDK CI uses), base `a7edeecfb` plus this patch:
- before the fix: `NeverWantedButInvoked:
rocksDBConfigManager.put("forbidden", "group", "null")` (invoked at
`clearForbidden`) and `NullPointerException` in
`testDecodeForbiddenIgnoresNullBody`
- after the fix: `RocksDBSubscriptionGroupManagerTest` — `Tests run: 6,
Failures: 0, Errors: 0`
- `mvn -B -ntp -pl broker package
-Dtest='RocksDBSubscriptionGroupManagerTest,RocksDBSubscriptionGroupManagerMigrationTest,ForbiddenTest,RocksdbGroupConfigTransferTest,SubscriptionGroupManagerTest'`:
`BUILD SUCCESS`, 26 tests / 0 failures, `You have 0 Checkstyle violations.`,
`BugInstance size is 0`
Disclosure: the full `broker` module suite does not complete on this machine
(it stalls in an unrelated transaction test around the 99th test class —
verified independent of this change), so I report the related classes plus the
static checks rather than a full-suite result.
This change was prepared with AI assistance (analysis, patch and test
drafting); the diff and every result above were reproduced locally by running
the module's tests against the base commit and the patched tree.
--
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]
