iloveeyjafjalla opened a new pull request, #11411:
URL: https://github.com/apache/rocketmq/pull/11411
### Which Issue(s) This PR Fixes
- Fixes #11410
### Brief Description
Cross-checking every `Option` a `tools` subcommand declares against the keys
the same command reads left three commands whose declared option set does not
match what `execute()` needs (the static-topic `-mf/--mapFile` mismatch was
already reported in #11381):
1. **`consumer`: `-i` is read but never declared.**
`ConsumerSubCommand.execute()` branches on `commandLine.hasOption('i')` and
reads `getOptionValue('i')` for the single-client query, but
`buildCommandlineOptions` only registered `-g` and `-s`, so the parser rejected
the invocation with `Unrecognized option: -i` and the `else` branch was
unreachable. The option is now declared (optional, wording copied from the
sibling `consumerStatus` command which already declares `-i/--clientId`).
2. **`resetMasterFlushOffset`: `-b/--brokerAddr` and `-o/--offset` become
required.** Both were declared `setRequired(false)` but `execute()` reads them
unconditionally with `.trim()` / `Long.parseLong(...trim())` and has no
fallback, so a missing option produced a `NullPointerException` (or, for a
non-numeric `-o`, a `NumberFormatException`) that `MQAdminStartup` prints as a
stack trace, with no usage information. Marking them required turns that into
the standard `Missing required option: -b` + help output that every other
command with mandatory arguments produces.
3. **`decodeMessageId`: `-i/--messageId` becomes required.** Same pattern:
declared optional, read unconditionally with `.trim()` before the first `try`,
so running the command without `-i` threw a `NullPointerException`.
No documented usage changes: each option is only made mandatory where
`execute()` has no alternative path, and `consumer -i` additionally restores
the per-client mode the command was written to support.
### How Did You Test This Change?
Three new test classes parse each command's own `Options` with a plain
`DefaultParser` (the pattern the module's existing tests use):
`ConsumerSubCommandTest`, `ResetMasterFlushOffsetSubCommandTest`,
`DecodeMessageIdCommondTest`. They assert the missing-argument behaviour the
user sees (parser rejects before `execute()` runs) and that the normal
invocation still parses.
Results on Temurin JDK 8 (the JDK CI uses), base `a7edeecfb` plus this patch:
| case | before the fix | after the fix |
|---|---|---|
| `consumer -g default-group -i cid_one -s` |
`org.apache.commons.cli.UnrecognizedOptionException: Unrecognized option: -i` |
parses; `-g/-i/-s` all readable |
| `resetMasterFlushOffset -o 100` (no `-b`) | no exception at parse time,
command proceeds to `NullPointerException` in `execute()` |
`org.apache.commons.cli.MissingRequiredOptionException` |
| `decodeMessageId` (no `-i`) | no exception at parse time,
`NullPointerException` in `execute()` |
`org.apache.commons.cli.MissingRequiredOptionException` |
- `mvn -B -ntp -pl tools test -Dtest=...` for the three classes: `Tests run:
6, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`.
- `mvn -B -ntp -pl tools package -Dtest='!QueryMsgTraceByIdSubCommandTest'`:
`BUILD SUCCESS`, 165 tests / 0 failures, `You have 0 Checkstyle violations.`,
spotbugs `check` passed.
- Disclosure: `QueryMsgTraceByIdSubCommandTest` is excluded from that run
because it fails on the unmodified base as well (`CODE: 208 query message by
key finished, but no message`, reproducible with the change stashed); it is
unrelated to these files, so I did not want its unrelated failure to stop the
reactor (`skipAfterFailureCount=1` is configured for this module).
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]
