[GitHub] [kafka] tombentley commented on a change in pull request #10649: KAFKA-12762: Use connection timeout when polling the network for new …

[GitBox <gi...@apache.org>]

tombentley commented on a change in pull request #10649:
URL: https://github.com/apache/kafka/pull/10649#discussion_r649793995



##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -536,7 +544,9 @@ static KafkaAdminClient createInternal(AdminClientConfig config, TimeoutProcesso
                 time,
                 true,
                 apiVersions,
-                logContext);
+                null,
+                logContext,
+                (hostResolver == null) ? new DefaultHostResolver() : hostResolver);

Review comment:
       Why the ternary here when you could've passed `new DefaultHostResolver()` on line 489?

##########
File path: clients/src/test/java/org/apache/kafka/clients/admin/AdminClientTestUtils.java
##########
@@ -102,4 +103,11 @@ public static DescribeTopicsResult describeTopicsResult(Map<String, TopicDescrip
     public static ListConsumerGroupOffsetsResult listConsumerGroupOffsetsResult(Map<TopicPartition, OffsetAndMetadata> offsets) {
         return new ListConsumerGroupOffsetsResult(KafkaFuture.completedFuture(offsets));
     }
+
+    /**
+     * Helper to create a KafkaAdminClient with a custom HostResolver

Review comment:
       ```suggestion
        * Helper to create a KafkaAdminClient with a custom HostResolver accessible to tests outside this package.
   ```

##########
File path: core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
##########
@@ -100,6 +101,19 @@ class PlaintextAdminIntegrationTest extends BaseAdminIntegrationTest {
     assertEquals(brokerStrs.mkString(","), nodeStrs.mkString(","))
   }
 
+  @Test
+  def testAdminClientHandlingBadIPWithoutTimeout(): Unit = {
+    val config = createConfig
+    config.put(AdminClientConfig.SOCKET_CONNECTION_SETUP_TIMEOUT_MS_CONFIG, "1000")
+    val returnBadAddressFirst = new HostResolver {
+      override def resolve(host: String): Array[InetAddress] = {
+        Array[InetAddress](InetAddress.getByName("10.200.20.100"), InetAddress.getByName(host))
+      }
+    }
+    client = AdminClientTestUtils.create(config, returnBadAddressFirst)
+    client.describeCluster().nodes().get()

Review comment:
       Maybe a comment that we're testing that this call actually returns (I assume it wouldn't have before you made the fix?)




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org