iloveeyjafjalla opened a new pull request, #11382:
URL: https://github.com/apache/rocketmq/pull/11382
### Which Issue(s) This PR Fixes
- Fixes #11381
### Brief Description
Both static-topic commands registered the map-file option under the key `mf`
and then read the key `f`:
```java
opt = new Option("mf", "mapFile", true, "The mapping data file name");
...
if (commandLine.hasOption("f")) { // registered nowhere
executeFromFile(commandLine, options, rpcHook);
```
so the documented `mqadmin updateStaticTopic -t T -b brokerA -qn 8 -mf
plan.json` never read the file and took the recompute
path instead — it rewrote the mapping computed from the cluster's current
state instead of applying the mapping the operator
supplied. (`getOptionValue('f')` inside `executeFromFile` would also have
NPE'd; `-f` is rejected by the parser.)
Getting to the file mode was blocked twice more, because the parser demanded
options that mode does not use: the
`-b|-c` option group and, for `updateStaticTopic`, `-qn/--totalQueueNum`
were `setRequired(true)`, while the file carries the
targets and the queue count — so `mqadmin updateStaticTopic -t T -mf
plan.json` printed the help via
`ServerUtil.parseCmdLine` and exited 1. Both recompute paths already
validate these options themselves.
One more thing surfaced once the path became reachable: `updateStaticTopic`
parsed `-fr/--forceReplace` and then passed a
literal `false` to `MQAdminUtils.updateTopicConfigMappingAll`.
The change reads the registered key in both commands, stops requiring
`-b|-c|-qn` for the from-file mode (the recompute mode
still validates them and prints the help), and passes the parsed `force`
through.
### How Did You Test This Change?
New tests `UpdateStaticTopicSubCommandTest` and
`RemappingStaticTopicSubCommandTest` (one per command):
- `testFromFileModeIsSelectedByTheMapFileOption` writes a real mapping file,
parses `-t … -b … -mf <file>` and asserts that
`loadMappingDetail` returns **the file's** mapping (epoch and broker).
- `testFromFileModeParsesWithoutTargetBrokers` asserts that `-t … -mf
<file>` parses at all (through a plain
`DefaultParser`: the mqadmin layer turns a parse error into `exit(1)`, so
the contract is that it parses).
I refactored `executeFromFile` to call a package-private
`loadMappingDetail(CommandLine)` first (pure extraction, behaviour
unchanged) so the "read the file" step is directly testable.
Results on Temurin JDK 8 (same JDK as CI), base `9ea2ccdc2` plus this patch:
- before the fix: `java.lang.NullPointerException` at `loadMappingDetail`
(the command reads an option nobody registers), and
the without-`-b` test fails with `Missing required option`
- after the fix: `UpdateStaticTopicSubCommandTest` +
`RemappingStaticTopicSubCommandTest` — 4 tests, 0 failures
- `mvn -B -ntp -pl tools package` with checkstyle + spotbugs enabled: `BUILD
SUCCESS`
- full `tools` suite: 157 tests, the only error is
`QueryMsgTraceByIdSubCommandTest`
(`CODE: 208 query message by key finished, but no message`), which fails
identically on the base commit with this change
stashed — pre-existing in this environment, unrelated to this PR
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]
