MINOR: Consistently apply timeout in group commands #20764
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR standardizes timeout handling in group commands by applying the --timeout option to all Admin client API calls, rather than only select operations. Previously, timeout configuration was inconsistently applied across different administrative operations.
Key changes:
- Updated timeout default from 5000ms to 30000ms and removed the restriction that it only applies to describe operations
- Modified all Admin client method calls to include Options objects with timeout configuration
- Removed unused logger instances and debug logging related to timeout option warnings
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| StreamsGroupCommandOptions.java | Updated timeout default to 30000ms, removed availability restriction, removed logger and associated debug warnings |
| ConsumerGroupCommandOptions.java | Removed logger and debug warnings for timeout option usage |
| StreamsGroupCommand.java | Added timeout to all Admin client calls via Options objects, removed unused collectAllTopics method |
| ShareGroupCommand.java | Added timeout to Admin client calls via Options objects |
| OffsetsUtils.java | Added timeout to describeTopics calls |
| StreamsGroupCommandTest.java | Updated mocks to expect Options parameters in Admin client calls |
| ShareGroupCommandTest.java | Updated mocks to expect Options parameters in Admin client calls |
| ConsumerGroupServiceTest.java | Updated mocks to expect Options parameters in describeTopics calls |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.ArgumentMatchers; | ||
| import org.mockito.MockedStatic; |
There was a problem hiding this 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.
| "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."); |
There was a problem hiding this comment.
This is evidently not true. Timeout is also applied in other actions.
| DescribeStreamsGroupsResult result = adminClient.describeStreamsGroups( | ||
| List.of(group), | ||
| new DescribeStreamsGroupsOptions().timeoutMs(opts.options.valueOf(opts.timeoutMsOpt).intValue())); | ||
| withTimeoutMs(new DescribeStreamsGroupsOptions())); |
There was a problem hiding this comment.
This is just using the same pattern as in other commands
| || topicName.matches(".+-KTABLE-FK-JOIN-SUBSCRIPTION-RESPONSE-\\d+-topic"); | ||
| } | ||
|
|
||
| List<String> collectAllTopics(String groupId) { |
There was a problem hiding this comment.
This was not used anywhere
| .describedAs("timeout (ms)") | ||
| .ofType(Long.class) | ||
| .defaultsTo(5000L); | ||
| .defaultsTo(30000L); |
There was a problem hiding this comment.
Inconsistent with other group types
| deleteOpt = parser.accepts("delete", DELETE_DOC); | ||
| deleteOffsetsOpt = parser.accepts("delete-offsets", DELETE_OFFSETS_DOC); | ||
| timeoutMsOpt = parser.accepts("timeout", TIMEOUT_MS_DOC) | ||
| .availableIf(describeOpt) |
There was a problem hiding this comment.
This timeout is anyways used in list groups, it seems.
| checkDescribeArgs(); | ||
| } else { | ||
| if (options.has(timeoutMsOpt)) | ||
| LOGGER.debug("Option " + timeoutMsOpt + " is applicable only when " + describeOpt + " is used."); |
|
@aliehsaeedii PTAL |
|
Thanks, @lucasbru, for cleaning up and ensuring consistency across the codebase. |
Group commands sometimes apply the timeout from --timeout, and sometimes don't. This change applies the timeout in every call to adminClient. Reviewers: Shivsundar R <[email protected]>, Andrew Schofield <[email protected]>, Alieh Saeedi <[email protected]>
4 participants
Group commands sometimes apply the timeout from --timeout, and sometimes
don't. This change applies the timeout in every call to adminClient.
Reviewers: Shivsundar R [email protected], Andrew Schofield
[email protected], Alieh Saeedi [email protected]