KAFKA-9251; Describing a non consumer group with the Admin API hangs forever#7763
Conversation
| context.future().complete(consumerGroupDescription); | ||
| } else { | ||
| context.future().completeExceptionally(new IllegalArgumentException( | ||
| "GroupId " + context.groupId() + " is not a consumer group.")); |
There was a problem hiding this comment.
For completeness, could we add the protocolType to this message?
There was a problem hiding this comment.
Yep. Thank for the suggestion.
| authorizedOperations); | ||
| context.future().complete(consumerGroupDescription); | ||
| } else { | ||
| context.future().completeExceptionally(new IllegalArgumentException( |
There was a problem hiding this comment.
What are your thoughts on having InvalidGroupIdException here?
In all regards the KafkaFuture will wrap it in an ExecutionException - I'm just wondering if user code could be checking whether the cause is a KafkaException.
There was a problem hiding this comment.
I thought about this and I felt like InvalidGroupIdException was inappropriate because the group is is valid but the type of the group is not accepted. Thus, IllegalArgumentException sounds better to me in this case. Does it make sense?
There was a problem hiding this comment.
Yeah it's not straightforward. Let's see what others think
There was a problem hiding this comment.
I'm ok with IllegalArgumentException. I think InvalidGroupIdException is typically used to indicate a groupId which is structurally invalid in some way (e.g. null).
|
|
||
| @Test | ||
| public void testDescribeNonConsumerGroups() throws Exception { | ||
| final HashMap<Integer, Node> nodes = new HashMap<>(); |
There was a problem hiding this comment.
Lots of duplication in these tests. No need to do it in this PR but I'm wondering if wrapping all the
final HashMap<Integer, Node> nodes = new HashMap<>();
nodes.put(0, new Node(0, "localhost", 8121));
final Cluster cluster =
new Cluster(
"mockClusterId",
nodes.values(),
Collections.<PartitionInfo>emptyList(),
Collections.<String>emptySet(),
Collections.<String>emptySet(), nodes.get(0));
try (AdminClientUnitTestEnv env = new AdminClientUnitTestEnv(cluster)) {
boilerplate into a single method that accepts a callable would look better in this test class
There was a problem hiding this comment.
Totally. If you don't mind, I will tackle this in a separate PR. I'd like to keep this one focus on the bugfix.
|
Nice catch! |
|
retest this please |
|
retest this please |
3 similar comments
|
retest this please |
|
retest this please |
|
retest this please |
…forever (#7763) If a non-consumer group is specified in `describeConsumerGroup`, the future will hang indefinitely because the future callback is never completed. This patch fixes the problem by completing the future exceptionally with an `IllegalArgumentException`. Reviewers: Stanislav Kozlovski <stanislav_kozlovski@outlook.com>, Jason Gustafson <jason@confluent.io>
…forever (#7763) If a non-consumer group is specified in `describeConsumerGroup`, the future will hang indefinitely because the future callback is never completed. This patch fixes the problem by completing the future exceptionally with an `IllegalArgumentException`. Reviewers: Stanislav Kozlovski <stanislav_kozlovski@outlook.com>, Jason Gustafson <jason@confluent.io>
Committer Checklist (excluded from commit message)