[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #15219: [Proxy & Client] Configure Netty DNS resolver to match JDK DNS caching setting, share DNS resolver instance in Proxy

[GitBox <gi...@apache.org>]

michaeljmarshall commented on code in PR #15219:
URL: https://github.com/apache/pulsar/pull/15219#discussion_r853261223


##########
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnectionPool.java:
##########
@@ -19,40 +19,18 @@
 package org.apache.pulsar.proxy.server;
 
 import io.netty.channel.EventLoopGroup;
-import java.io.IOException;
-import java.util.concurrent.ExecutionException;
+import io.netty.resolver.dns.DnsNameResolver;
+import java.util.Optional;
 import java.util.function.Supplier;
 import org.apache.pulsar.client.api.PulsarClientException;
 import org.apache.pulsar.client.impl.ClientCnx;
 import org.apache.pulsar.client.impl.ConnectionPool;
 import org.apache.pulsar.client.impl.conf.ClientConfigurationData;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 public class ProxyConnectionPool extends ConnectionPool {
     public ProxyConnectionPool(ClientConfigurationData clientConfig, EventLoopGroup eventLoopGroup,
-            Supplier<ClientCnx> clientCnxSupplier) throws PulsarClientException {
-        super(clientConfig, eventLoopGroup, clientCnxSupplier);
+                               Supplier<ClientCnx> clientCnxSupplier, DnsNameResolver dnsNameResolver)
+            throws PulsarClientException {
+        super(clientConfig, eventLoopGroup, clientCnxSupplier, Optional.of(dnsNameResolver));

Review Comment:
   Nit: is it worth removing this class and adding an additional constructor for the `ConnectionPool`?



##########
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnectionPool.java:
##########
@@ -19,40 +19,18 @@
 package org.apache.pulsar.proxy.server;
 
 import io.netty.channel.EventLoopGroup;
-import java.io.IOException;
-import java.util.concurrent.ExecutionException;
+import io.netty.resolver.dns.DnsNameResolver;
+import java.util.Optional;
 import java.util.function.Supplier;
 import org.apache.pulsar.client.api.PulsarClientException;
 import org.apache.pulsar.client.impl.ClientCnx;
 import org.apache.pulsar.client.impl.ConnectionPool;
 import org.apache.pulsar.client.impl.conf.ClientConfigurationData;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 public class ProxyConnectionPool extends ConnectionPool {
     public ProxyConnectionPool(ClientConfigurationData clientConfig, EventLoopGroup eventLoopGroup,
-            Supplier<ClientCnx> clientCnxSupplier) throws PulsarClientException {
-        super(clientConfig, eventLoopGroup, clientCnxSupplier);
+                               Supplier<ClientCnx> clientCnxSupplier, DnsNameResolver dnsNameResolver)
+            throws PulsarClientException {
+        super(clientConfig, eventLoopGroup, clientCnxSupplier, Optional.of(dnsNameResolver));
     }
-
-    @Override
-    public void close() throws IOException {
-        log.info("Closing ProxyConnectionPool.");
-        pool.forEach((address, clientCnxPool) -> {
-            if (clientCnxPool != null) {
-                clientCnxPool.forEach((identifier, clientCnx) -> {
-                    if (clientCnx != null && clientCnx.isDone()) {
-                        try {
-                            clientCnx.get().close();
-                        } catch (InterruptedException | ExecutionException e) {
-                            log.error("Unable to close get client connection future.", e);
-                        }
-                    }

Review Comment:
   Interestingly, it looks like this logic might have resulted in a `clientCnx` leak, so removing this could be a positive side effect. It differs from the `super.close()` in that the `ConnectionPool` implementation calls `future.thenAccept(ClientCnx::close);` if the future is not done.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org