Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2334,7 +2334,7 @@ private Map<String, KafkaFuture<TopicDescription>> handleDescribeTopicsByNamesWi
}

// First, we need to retrieve the node info.
DescribeClusterResult clusterResult = describeCluster();
DescribeClusterResult clusterResult = describeCluster(new DescribeClusterOptions().timeoutMs(options.timeoutMs()));
clusterResult.nodes().whenComplete(
(nodes, exception) -> {
if (exception != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11668,4 +11668,26 @@ private static StreamsGroupDescribeResponseData makeFullStreamsGroupDescribeResp
.setAssignmentEpoch(1));
return data;
}

@Test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add @Timeout(30) to this unit test?

public void testDescribeTopicsTimeoutWhenNoBrokerResponds() throws Exception {
try (AdminClientUnitTestEnv env = new AdminClientUnitTestEnv(
mockCluster(1, 0),
AdminClientConfig.RETRIES_CONFIG, "0",
AdminClientConfig.REQUEST_TIMEOUT_MS_CONFIG, "30000")) {
env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());

// Not using prepareResponse is equivalent to "no brokers respond".
long start = System.currentTimeMillis();
DescribeTopicsResult result = env.adminClient().describeTopics(Collections.singletonList("test-topic"), new DescribeTopicsOptions().timeoutMs(200));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List.of

Map<String, KafkaFuture<TopicDescription>> topicDescriptionMap = result.topicNameValues();
KafkaFuture<TopicDescription> topicDescription = topicDescriptionMap.get("test-topic");
ExecutionException exception = assertThrows(ExecutionException.class, () -> topicDescription.get());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertThrows(ExecutionException.class, topicDescription::get);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these suggestions, update it.

// Duration should be greater than or equal to 200 ms but less than 30000 ms.
long duration = System.currentTimeMillis() - start;

assertInstanceOf(TimeoutException.class, exception.getCause());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertInstanceOf(TimeoutException.class, exception.getCause());
assertEquals(TimeoutException.class, exception.getCause().getClass());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a common debate between assertInstanceOf and assertEquals. The difference is that assertInstanceOf allows subtypes, while assertEquals does not.

In this case, I prefer assertInstanceOf since we may introduce different varieties of TimeoutException in the future. Using assertInstanceOf provides more flexibility than assertEquals, and it also does not violate the documentation

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explanation!

assertTrue(duration >= 150L && duration < 2000L);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pardon me, how do you come up with 2000L?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this comment, update it.

}
}
}
Loading