You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "regarmukesh3g (via GitHub)" <gi...@apache.org> on 2026/09/03 16:22:23 UTC
[PR] MINOR: Fix GroupsCommand exiting from execute() and masking failures [kafka]
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix GroupsCommand exiting from execute() and masking failures [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23349:
URL: https://github.com/apache/kafka/pull/23349#issuecomment-5738951938
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix GroupsCommand exiting from execute() and masking failures [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #23349:
URL: https://github.com/apache/kafka/pull/23349#discussion_r4349297171
##########
tools/src/test/java/org/apache/kafka/tools/GroupsCommandTest.java:
##########
@@ -463,6 +467,43 @@ public void testListGroupsFailsWithException() {
)));
}
+ @Test
+ public void testMainNoExitReturnsNonZeroWithoutExitingOnFailure() throws Exception {
+ // An invalid security protocol makes the admin client creation inside execute() fail.
+ // mainNoExit must report that failure by returning a non-zero code, and execute() must
+ // not terminate the JVM itself.
+ assertEquals(1, GroupsCommand.mainNoExit(
+ "--bootstrap-server", bootstrapServer,
+ "--list",
+ "--command-config", invalidCommandConfig()));
+ assertFalse(exitProcedure.hasExited(), "execute() must not call Exit.exit(); only main() may exit");
+ }
+
+ @Test
+ public void testExecuteDoesNotExitOnFailure() throws Exception {
+ // execute() propagates the failure to its caller rather than terminating the JVM.
+ String commandConfig = invalidCommandConfig();
+ assertThrows(Exception.class, () -> GroupsCommand.execute(
+ "--bootstrap-server", bootstrapServer,
+ "--list",
+ "--command-config", commandConfig));
+ assertFalse(exitProcedure.hasExited(), "execute() must not call Exit.exit(); only main() may exit");
+ }
+
+ /**
+ * Writes a command config file which parses correctly but makes the admin client creation
+ * inside {@link GroupsCommand#execute} fail.
+ */
+ private String invalidCommandConfig() throws IOException {
Review Comment:
Could you use `ToolsTestUtils.tempPropertiesFile` instead like `ConfigCommandTest`?
##########
tools/src/main/java/org/apache/kafka/tools/GroupsCommand.java:
##########
@@ -57,8 +56,7 @@ static int mainNoExit(String... args) {
execute(args);
return 0;
} catch (Throwable e) {
- System.err.println(e.getMessage());
Review Comment:
There's a behaviour change here. Formerly, the exception information was written to stderr, but now it's stdout.
##########
tools/src/test/java/org/apache/kafka/tools/GroupsCommandTest.java:
##########
@@ -463,6 +467,43 @@ public void testListGroupsFailsWithException() {
)));
}
+ @Test
+ public void testMainNoExitReturnsNonZeroWithoutExitingOnFailure() throws Exception {
+ // An invalid security protocol makes the admin client creation inside execute() fail.
+ // mainNoExit must report that failure by returning a non-zero code, and execute() must
+ // not terminate the JVM itself.
+ assertEquals(1, GroupsCommand.mainNoExit(
+ "--bootstrap-server", bootstrapServer,
+ "--list",
+ "--command-config", invalidCommandConfig()));
+ assertFalse(exitProcedure.hasExited(), "execute() must not call Exit.exit(); only main() may exit");
+ }
+
+ @Test
+ public void testExecuteDoesNotExitOnFailure() throws Exception {
+ // execute() propagates the failure to its caller rather than terminating the JVM.
+ String commandConfig = invalidCommandConfig();
+ assertThrows(Exception.class, () -> GroupsCommand.execute(
Review Comment:
Can you assert on the specific exception class please?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix GroupsCommand exiting from execute() and masking failures [kafka]
Posted by "regarmukesh3g (via GitHub)" <gi...@apache.org>.
regarmukesh3g commented on PR #23349:
URL: https://github.com/apache/kafka/pull/23349#issuecomment-5585064444
@m1a2st @chia7712 would one of you be willing to take a look? You are both frequent reviewers of the `tools` module.
Some context that did not belong in the commit message. `GroupsCommand.execute()` calls `Exit.exit(exitCode)` in a `finally` block, which no other tool in the package does — `FeatureCommand`, `DelegationTokenCommand`, `MetadataQuorumCommand`, `GetOffsetShell` and `LeaderElectionCommand` all call `Exit.exit` exactly once from `main()` and let `execute()` propagate failures to `mainNoExit()`.
The user-visible effect is that under a replaced exit procedure the exception is swallowed and `mainNoExit()` returns 0 for a command that actually failed. The two added tests fail without the fix:
```
testMainNoExitReturnsNonZeroWithoutExitingOnFailure() :: expected: <1> but was: <0>
testExecuteDoesNotExitOnFailure() :: Expected java.lang.Exception to be thrown, but nothing was thrown.
```
The full CI build has not run because the PR lacks the `ci-approved` label, which only a committer can apply.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix GroupsCommand exiting from execute() and masking failures [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23349:
URL: https://github.com/apache/kafka/pull/23349#issuecomment-6331873970
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org