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:
us...@infra.apache.org
