divijvaidya commented on code in PR #14110:
URL: https://github.com/apache/kafka/pull/14110#discussion_r1278956423
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -2354,6 +2354,55 @@ public void testDeleteRecords() throws Exception {
}
}
+ @Test
+ public void testDescribeTopicsByIds() {
+ try (AdminClientUnitTestEnv env = new
AdminClientUnitTestEnv(mockCluster(4, 0),
+ AdminClientConfig.RETRIES_CONFIG, "2")) {
Review Comment:
Just use `try (AdminClientUnitTestEnv env = mockClientEnv()) {` maybe?
Setting cluster to 4 nodes and 2 retries does't sound relevant to this test
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -2354,6 +2354,55 @@ public void testDeleteRecords() throws Exception {
}
}
+ @Test
+ public void testDescribeTopicsByIds() {
+ try (AdminClientUnitTestEnv env = new
AdminClientUnitTestEnv(mockCluster(4, 0),
+ AdminClientConfig.RETRIES_CONFIG, "2")) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+
+ // Valid ID
Review Comment:
we have added a case for valid Id, another for unrepresentable id and
perhaps we have have one more for id which does not exist in the broker? Can
you please add the third case?
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -2354,6 +2354,55 @@ public void testDeleteRecords() throws Exception {
}
}
+ @Test
+ public void testDescribeTopicsByIds() {
+ try (AdminClientUnitTestEnv env = new
AdminClientUnitTestEnv(mockCluster(4, 0),
+ AdminClientConfig.RETRIES_CONFIG, "2")) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+
+ // Valid ID
+ Uuid topicId = Uuid.randomUuid();
+ String topicName = "test-topic";
+ Node leader = env.cluster().nodes().get(0);
+ MetadataResponse.PartitionMetadata partitionMetadata = new
MetadataResponse.PartitionMetadata(
+ Errors.NONE,
+ new TopicPartition(topicName, 0),
+ Optional.of(leader.id()),
+ Optional.of(10),
+ singletonList(leader.id()),
+ singletonList(leader.id()),
+ singletonList(leader.id()));
+ env.kafkaClient().prepareResponse(RequestTestUtils
+ .metadataResponse(
+ env.cluster().nodes(),
+ env.cluster().clusterResource().clusterId(),
+ env.cluster().controller().id(),
+ singletonList(new
MetadataResponse.TopicMetadata(Errors.NONE, topicName, topicId, false,
+ singletonList(partitionMetadata),
MetadataResponse.AUTHORIZED_OPERATIONS_OMITTED))));
+ TopicCollection.TopicIdCollection topicIds =
TopicCollection.ofTopicIds(
+ singletonList(topicId));
+ try {
+ DescribeTopicsResult result = env.adminClient().describeTopics(
+ TopicCollection.ofTopicIds(singletonList(topicId)));
+ Map<Uuid, TopicDescription> allTopicIds =
result.allTopicIds().get();
+ assertEquals(topicName, allTopicIds.get(topicId).name());
+ } catch (Exception e) {
+ fail("describe with valid topicId should not fail", e);
+ }
+
+ // Invalid ID
+ try {
+ DescribeTopicsResult result = env.adminClient().describeTopics(
+
TopicCollection.ofTopicIds(singletonList(Uuid.ZERO_UUID)));
+ result.allTopicIds().get();
Review Comment:
you can also choose to use `TestUtils.assertFutureError`
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -2354,6 +2354,55 @@ public void testDeleteRecords() throws Exception {
}
}
+ @Test
+ public void testDescribeTopicsByIds() {
+ try (AdminClientUnitTestEnv env = new
AdminClientUnitTestEnv(mockCluster(4, 0),
+ AdminClientConfig.RETRIES_CONFIG, "2")) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+
+ // Valid ID
+ Uuid topicId = Uuid.randomUuid();
+ String topicName = "test-topic";
+ Node leader = env.cluster().nodes().get(0);
+ MetadataResponse.PartitionMetadata partitionMetadata = new
MetadataResponse.PartitionMetadata(
+ Errors.NONE,
+ new TopicPartition(topicName, 0),
+ Optional.of(leader.id()),
+ Optional.of(10),
+ singletonList(leader.id()),
+ singletonList(leader.id()),
+ singletonList(leader.id()));
+ env.kafkaClient().prepareResponse(RequestTestUtils
+ .metadataResponse(
+ env.cluster().nodes(),
+ env.cluster().clusterResource().clusterId(),
+ env.cluster().controller().id(),
+ singletonList(new
MetadataResponse.TopicMetadata(Errors.NONE, topicName, topicId, false,
+ singletonList(partitionMetadata),
MetadataResponse.AUTHORIZED_OPERATIONS_OMITTED))));
+ TopicCollection.TopicIdCollection topicIds =
TopicCollection.ofTopicIds(
+ singletonList(topicId));
+ try {
+ DescribeTopicsResult result = env.adminClient().describeTopics(
+ TopicCollection.ofTopicIds(singletonList(topicId)));
Review Comment:
could we use `topicIds` local variable defined above at line 2382?
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -2354,6 +2354,55 @@ public void testDeleteRecords() throws Exception {
}
}
+ @Test
+ public void testDescribeTopicsByIds() {
+ try (AdminClientUnitTestEnv env = new
AdminClientUnitTestEnv(mockCluster(4, 0),
+ AdminClientConfig.RETRIES_CONFIG, "2")) {
+ env.kafkaClient().setNodeApiVersions(NodeApiVersions.create());
+
+ // Valid ID
+ Uuid topicId = Uuid.randomUuid();
+ String topicName = "test-topic";
+ Node leader = env.cluster().nodes().get(0);
+ MetadataResponse.PartitionMetadata partitionMetadata = new
MetadataResponse.PartitionMetadata(
+ Errors.NONE,
+ new TopicPartition(topicName, 0),
+ Optional.of(leader.id()),
+ Optional.of(10),
+ singletonList(leader.id()),
+ singletonList(leader.id()),
+ singletonList(leader.id()));
+ env.kafkaClient().prepareResponse(RequestTestUtils
+ .metadataResponse(
+ env.cluster().nodes(),
+ env.cluster().clusterResource().clusterId(),
+ env.cluster().controller().id(),
+ singletonList(new
MetadataResponse.TopicMetadata(Errors.NONE, topicName, topicId, false,
+ singletonList(partitionMetadata),
MetadataResponse.AUTHORIZED_OPERATIONS_OMITTED))));
+ TopicCollection.TopicIdCollection topicIds =
TopicCollection.ofTopicIds(
+ singletonList(topicId));
+ try {
+ DescribeTopicsResult result = env.adminClient().describeTopics(
+ TopicCollection.ofTopicIds(singletonList(topicId)));
+ Map<Uuid, TopicDescription> allTopicIds =
result.allTopicIds().get();
+ assertEquals(topicName, allTopicIds.get(topicId).name());
+ } catch (Exception e) {
+ fail("describe with valid topicId should not fail", e);
+ }
+
+ // Invalid ID
+ try {
+ DescribeTopicsResult result = env.adminClient().describeTopics(
+
TopicCollection.ofTopicIds(singletonList(Uuid.ZERO_UUID)));
+ result.allTopicIds().get();
+ fail("describe with Uuid.ZERO_UUID should throw exception");
+ } catch (Exception e) {
+ assertEquals("The given topic id 'AAAAAAAAAAAAAAAAAAAAAA'
cannot be represented in a request.",
Review Comment:
please also assert exception type to be `InvalidTopicException`
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
