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, but while working on it, I realized
that 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
