MINOR: describeTopics should pass the timeout to the describeCluster call #20375
Conversation
There was a problem hiding this comment.
@jim0987795064 could you please add a integration test for it?
@chia7712, Thanks for this suggestion, I've added a test which is used to make sure that describeCluster throws an exception when no broker responds. |
|
|
||
| // Not using prepareResponse is equivalent to "no brokers respond". | ||
| long start = System.currentTimeMillis(); | ||
| ExecutionException exception = assertThrows(ExecutionException.class, () -> env.adminClient().describeCluster(new DescribeClusterOptions().timeoutMs(200)).clusterId().get()); |
There was a problem hiding this comment.
the API which should be tested is describeTopics, right?
There was a problem hiding this comment.
@chia7712
Thanks for pointing this out, I've changed tested API to describeTopics.
| // Duration should be greater than or equal to 200 ms but less than 30000 ms. | ||
| long duration = System.currentTimeMillis() - start; | ||
|
|
||
| assertTrue(exception.getCause() instanceof TimeoutException); |
There was a problem hiding this comment.
assertInstanceOf(TimeoutException.class, exception.getCause());
| @@ -11668,4 +11668,26 @@ private static StreamsGroupDescribeResponseData makeFullStreamsGroupDescribeResp | |||
| .setAssignmentEpoch(1)); | |||
| return data; | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Could you please add @Timeout(30) to this unit test?
| DescribeTopicsResult result = env.adminClient().describeTopics(Collections.singletonList("test-topic"), new DescribeTopicsOptions().timeoutMs(200)); | ||
| Map<String, KafkaFuture<TopicDescription>> topicDescriptionMap = result.topicNameValues(); | ||
| KafkaFuture<TopicDescription> topicDescription = topicDescriptionMap.get("test-topic"); | ||
| ExecutionException exception = assertThrows(ExecutionException.class, () -> topicDescription.get()); |
There was a problem hiding this comment.
assertThrows(ExecutionException.class, topicDescription::get);
There was a problem hiding this comment.
Thanks for these suggestions, update it.
| @@ -11668,4 +11668,26 @@ private static StreamsGroupDescribeResponseData makeFullStreamsGroupDescribeResp | |||
| .setAssignmentEpoch(1)); | |||
| return data; | |||
| } | |||
|
|
|||
| @Test @Timeout(30) | |||
There was a problem hiding this comment.
Please don't put these two annotations in the same line.
|
|
||
| // 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)); |
| long duration = System.currentTimeMillis() - start; | ||
|
|
||
| assertInstanceOf(TimeoutException.class, exception.getCause()); | ||
| assertTrue(duration >= 150L && duration < 2000L); |
There was a problem hiding this comment.
Pardon me, how do you come up with 2000L?
There was a problem hiding this comment.
Thanks for this comment, 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()); |
There was a problem hiding this comment.
| assertInstanceOf(TimeoutException.class, exception.getCause()); | |
| assertEquals(TimeoutException.class, exception.getCause().getClass()); |
There was a problem hiding this comment.
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
This PR ensures that describeTopics correctly propagates its timeoutMs
setting to the underlying describeCluster call. Integration tests were
added to verify that the API now fails with a TimeoutException when
brokers do not respond within the configured timeout.
Reviewers: Ken Huang [email protected], TengYao Chi
[email protected], Chia-Ping Tsai [email protected]