bob-barrett commented on a change in pull request #9902:
URL: https://github.com/apache/kafka/pull/9902#discussion_r569992543
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -106,8 +106,9 @@ public static ChannelBuilder
createChannelBuilder(AbstractConfig config, Time ti
clientSaslMechanism, time, true, logContext);
}
- static List<InetAddress> resolve(String host, ClientDnsLookup
clientDnsLookup) throws UnknownHostException {
- InetAddress[] addresses = InetAddress.getAllByName(host);
+ static List<InetAddress> resolve(String host, ClientDnsLookup
clientDnsLookup,
+ HostResolver hostResolver) throws
UnknownHostException {
Review comment:
@dajac I started making this change, and I don't think I like requiring
the `HostResolver` to understand the `client.dns.lookup` config. In the same
way that `MockTime` is only used to mock Java built-in methods, I think the
`HostResolver` should only be responsible for the DNS resolution (either real
or mocked), and that `ClientUtils` should be responsible for understanding the
Kafka application behavior. That way, tests that mock DNS still test the actual
`ClientDnsLookup` behavior, rather than relying on both the default resolver
and any mocked resolvers handle it correctly. It also means that if we ever add
an option for `ClientDnsLookup` or change its behavior, we don't need to
remember to update all `HostResolver` implementations.
----------------------------------------------------------------
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
