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.
##########
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.
##########
File path: clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java
##########
@@ -81,6 +85,19 @@
private final NetworkClient clientWithNoExponentialBackoff =
createNetworkClient(reconnectBackoffMsTest);
private final NetworkClient clientWithStaticNodes =
createNetworkClientWithStaticNodes();
private final NetworkClient clientWithNoVersionDiscovery =
createNetworkClientWithNoVersionDiscovery();
+ private ArrayList<InetAddress> initialAddresses = new
ArrayList<>(Arrays.asList(
Review comment:
Catching the `UnknownHostException` makes it a bit annoying, I threw it
in a static block to make the compiler happy. Let me know if you think there's
something neater.
##########
File path: clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java
##########
@@ -81,6 +85,19 @@
private final NetworkClient clientWithNoExponentialBackoff =
createNetworkClient(reconnectBackoffMsTest);
private final NetworkClient clientWithStaticNodes =
createNetworkClientWithStaticNodes();
private final NetworkClient clientWithNoVersionDiscovery =
createNetworkClientWithNoVersionDiscovery();
+ private ArrayList<InetAddress> initialAddresses = new
ArrayList<>(Arrays.asList(
Review comment:
Catching the `UnknownHostException` makes it a bit annoying, I threw it
in a static block to make the compiler happy. Let me know if you think there's
something cleaner.
----------------------------------------------------------------
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
