dajac commented on a change in pull request #9902:
URL: https://github.com/apache/kafka/pull/9902#discussion_r569299977
##########
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:
This method and its usages look a bit weird to me now. It resolves an
`host` based on `clientDnsLookup` and a `hostResolver` which resolves an `host`
at its turn. We also have to pass the `HostResolver` wherever it is called now.
I wonder if we should avoid this level of indirection here and directly
encapsulate the entire logic in the `HostResolver`.
`HostResolver` would look like this:
```
public interface HostResolver {
List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup)
throws UnknownHostException;
}
```
In `NodeConnectionState#currentAddress`, we could replace `addresses =
ClientUtils.resolve(host, clientDnsLookup, hostResolver)` by `addresses =
hostResolver.resolve(host, clientDnsLookup)`.
The `DefaultHostResolver` would do what `ClientUtils.resolve` does today and
we can mock it in tests like you did.
Is this something that you have considered?
----------------------------------------------------------------
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
