You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "lucasbru (via GitHub)" <gi...@apache.org> on 2025/10/24 12:48:49 UTC
[PR] MINOR: Consistently apply timeout in group commands [kafka]
lucasbru opened a new pull request, #20764:
URL: https://github.com/apache/kafka/pull/20764
Group commands sometimes apply the timeout from --timeout, and sometimes don't. This change applies the timeout in every call to adminClient.
--
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: Consistently apply timeout in group commands [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on PR #20764:
URL: https://github.com/apache/kafka/pull/20764#issuecomment-3443196794
@aliehsaeedii
--
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: Consistently apply timeout in group commands [kafka]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #20764:
URL: https://github.com/apache/kafka/pull/20764#discussion_r2460290125
##########
tools/src/test/java/org/apache/kafka/tools/streams/StreamsGroupCommandTest.java:
##########
@@ -47,7 +50,6 @@
import org.apache.kafka.test.TestUtils;
import org.junit.jupiter.api.Test;
-import org.mockito.ArgumentMatchers;
import org.mockito.MockedStatic;
Review Comment:
The import for `ArgumentMatchers` was removed but `anyCollection()`, `anyMap()`, `anyString()`, and `eq()` are now imported individually. This is inconsistent with the continued use of `any()` which still requires `ArgumentMatchers` to be in scope. Consider using `ArgumentMatchers.any()` or adding individual imports for all matcher methods.
--
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: Consistently apply timeout in group commands [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #20764:
URL: https://github.com/apache/kafka/pull/20764#discussion_r2460309662
##########
tools/src/main/java/org/apache/kafka/tools/streams/StreamsGroupCommandOptions.java:
##########
@@ -144,11 +140,10 @@ public StreamsGroupCommandOptions(String[] args) {
deleteOpt = parser.accepts("delete", DELETE_DOC);
deleteOffsetsOpt = parser.accepts("delete-offsets", DELETE_OFFSETS_DOC);
timeoutMsOpt = parser.accepts("timeout", TIMEOUT_MS_DOC)
- .availableIf(describeOpt)
.withRequiredArg()
.describedAs("timeout (ms)")
.ofType(Long.class)
- .defaultsTo(5000L);
+ .defaultsTo(30000L);
Review Comment:
Inconsistent with other group types
--
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: Consistently apply timeout in group commands [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #20764:
URL: https://github.com/apache/kafka/pull/20764#discussion_r2460305726
##########
tools/src/main/java/org/apache/kafka/tools/streams/StreamsGroupCommand.java:
##########
@@ -947,22 +975,6 @@ public static boolean matchesInternalTopicFormat(final String topicName) {
|| topicName.matches(".+-KTABLE-FK-JOIN-SUBSCRIPTION-RESPONSE-\\d+-topic");
}
- List<String> collectAllTopics(String groupId) {
Review Comment:
This was not used anywhere
--
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: Consistently apply timeout in group commands [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #20764:
URL: https://github.com/apache/kafka/pull/20764#discussion_r2460316576
##########
tools/src/main/java/org/apache/kafka/tools/streams/StreamsGroupCommandOptions.java:
##########
@@ -144,11 +140,10 @@ public StreamsGroupCommandOptions(String[] args) {
deleteOpt = parser.accepts("delete", DELETE_DOC);
deleteOffsetsOpt = parser.accepts("delete-offsets", DELETE_OFFSETS_DOC);
timeoutMsOpt = parser.accepts("timeout", TIMEOUT_MS_DOC)
- .availableIf(describeOpt)
Review Comment:
This timeout is anyways used in list groups, it seems.
##########
tools/src/main/java/org/apache/kafka/tools/streams/StreamsGroupCommandOptions.java:
##########
@@ -214,9 +209,6 @@ void checkArgs() {
if (options.has(describeOpt)) {
checkDescribeArgs();
- } else {
- if (options.has(timeoutMsOpt))
- LOGGER.debug("Option " + timeoutMsOpt + " is applicable only when " + describeOpt + " is used.");
Review Comment:
not true
--
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: Consistently apply timeout in group commands [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #20764:
URL: https://github.com/apache/kafka/pull/20764#discussion_r2460301469
##########
tools/src/main/java/org/apache/kafka/tools/streams/StreamsGroupCommand.java:
##########
@@ -279,7 +284,7 @@ public void describeGroups() throws ExecutionException, InterruptedException {
StreamsGroupDescription getDescribeGroup(String group) throws ExecutionException, InterruptedException {
DescribeStreamsGroupsResult result = adminClient.describeStreamsGroups(
List.of(group),
- new DescribeStreamsGroupsOptions().timeoutMs(opts.options.valueOf(opts.timeoutMsOpt).intValue()));
+ withTimeoutMs(new DescribeStreamsGroupsOptions()));
Review Comment:
This is just using the same pattern as in other commands
--
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: Consistently apply timeout in group commands [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru merged PR #20764:
URL: https://github.com/apache/kafka/pull/20764
--
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: Consistently apply timeout in group commands [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #20764:
URL: https://github.com/apache/kafka/pull/20764#discussion_r2460296330
##########
tools/src/main/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommandOptions.java:
##########
@@ -230,9 +226,6 @@ void checkArgs() {
if (options.has(stateOpt) && options.valueOf(stateOpt) != null)
CommandLineUtils.printUsageAndExit(parser,
"Option " + describeOpt + " does not take a value for " + stateOpt);
- } else {
- if (options.has(timeoutMsOpt))
- LOGGER.debug("Option " + timeoutMsOpt + " is applicable only when " + describeOpt + " is used.");
Review Comment:
This is evidently not true. Timeout is also applied in other actions.
--
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: Consistently apply timeout in group commands [kafka]
Posted by "aliehsaeedii (via GitHub)" <gi...@apache.org>.
aliehsaeedii commented on PR #20764:
URL: https://github.com/apache/kafka/pull/20764#issuecomment-3443695144
Thanks, @lucasbru, for cleaning up and ensuring consistency across the codebase.
--
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