regarmukesh3g opened a new pull request, #23349:
URL: https://github.com/apache/kafka/pull/23349
## Problem
`GroupsCommand.execute()` calls `Exit.exit(exitCode)` in a `finally` block:
```java
} catch (Throwable t) {
printException(t);
exitCode = 1;
} finally {
Exit.exit(exitCode);
}
```
Every other tool in this package (`FeatureCommand`,
`DelegationTokenCommand`, `MetadataQuorumCommand`, `GetOffsetShell`,
`LeaderElectionCommand`) calls `Exit.exit` exactly once, from `main()`, and
lets `execute()` propagate failures to `mainNoExit()`. `GroupsCommand` is the
only one that exits from inside `execute()`.
This has two consequences:
- **Under the default exit procedure**, `execute()` terminates the JVM
directly, so `mainNoExit()` never returns and the exit code it computes is
unreachable.
- **Under a replaced exit procedure** (for example the `MockExitProcedure`
used in tests, which records the code and returns), `execute()` swallows the
exception and returns normally, so `mainNoExit()` reports **success with code 0
for a command that actually failed**.
The `mainNoExit()` / `execute()` pair exists precisely so the exit code can
be observed without killing the JVM, and this defeats it.
## Fix
Remove the `Exit.exit()` call and the `exitCode` bookkeeping from
`execute()`, letting the exception propagate so `mainNoExit()` maps it to a
non-zero code and `main()` passes that to `Exit.exit()`. `ExecutionException`
is still unwrapped so the underlying cause is what surfaces to the user. Error
reporting in `mainNoExit()` now goes through `printException()` so the message
and stack trace are rendered the same way on both paths.
## Testing
Added two tests covering `mainNoExit()` and `execute()` on the failure path,
which had no coverage — the gap that let this go unnoticed. Both fail before
this change:
```
testMainNoExitReturnsNonZeroWithoutExitingOnFailure() :: expected: <1> but
was: <0>
testExecuteDoesNotExitOnFailure() :: Expected java.lang.Exception to be
thrown, but nothing was thrown.
```
and pass after it. The tests make the admin client creation *inside* the
`try` block fail (via an invalid security protocol in the command config), so
they exercise the affected path rather than failing earlier during option
parsing.
`GroupsCommandTest` passes in full (33 tests), as do
`LeaderElectionCommandTest` and `TopicCommandTest` (146 tests total).
`./gradlew :tools:checkstyleMain :tools:checkstyleTest :tools:spotlessCheck` is
clean.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01GvbzZgK7eH63iPswf8tV6v
--
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]
