Repository: netty/netty
Issue: Proposal: DnsAddressResolverGroup to use injectable DnsNameResolverBuilder

### Expected behavior

It should be possible to create `DnsAddressResolverGroup` having `DnsNameResolverBuilder` ready-to-go.

### Actual behavior

Right now, if I want setup custom settings for `DnsAddressResolver` I need to sub-class `DnsAddressResolverGroup` and override `newNameResolver` [method](https://github.com/netty/netty/blob/06dcca1dbcc048b6ce7d954e4ecdf1d3ed6af034/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolverGroup.java#L97) as it creates new `DnsNameResolverBuilder` with "almost" default settings internally.

Ideally, I want to instantiate `DnsNameResolverBuilder` by my own and pass it to the `DnsAddressResolverGroup` as an argument. More than that, I don't need `channelFactory` and `nameServerProvider` being carried over `DnsAddressResolverGroup`: I can set them directly to the `DnsNameResolverBuilder` instance.

It probably would be even easier to create a new `AddressResolverGroup` implementation to work this way, but `InflightNameResolver` is not allowed to be used from the outside of the package, so if the user wants to benefit from it, it should follow `DnsAddressResolverGroup` API and override `newNameResolver`.

### Netty version

4.1.*

### Proposal

There're 2 options here:

1. Upgrade `DnsAddressResolverGroup` with a new constructor 

```java
public DnsAddressResolverGroup(DnsNameResolverBuilder builder);
```

and appropriate checker if we have `DnsNameResolverBuilder` or we need to create a new one.

2. Create a new `AddressResolverGroup` implementation, like `PluggableDnsAddressResolverGroup` (? naming is hard).

What do you think? Happy to create a PR if there are no objections here.

Issue comments:
Fixed by https://github.com/netty/netty/pull/7793

Pull request review comments:
- build -> build a
- DnsNameResolver -> DnsNameResolver. (dot)
@trustin 🤔shoud other 2 `IllegalStateException` be updated with a trailing dot? 
This won't work.   If someone makes a DnsNameResolverBuilder and then calls build, and then subsequently modifies the builder, the channelFactory of DnsAddressResolverGroup will change. 

That would be unexpected.
@carl-mastrangelo Not sure I follow. If you passed `DnsNameResolverBuilder` to create a `DnsAddressResolverGroup`, what you would expect from changing `channelFactory` on the builder you've used? 
@trustin Fixed the message.
I wouldn't expect the DnsAddressResolverGroup to use a different channel factory if the DnsNameResolverBuilder changed channelFactory.    That is currently possible as is.
@carl-mastrangelo Would a deep copy of the builder meet your expectations? This will prevent mutations propagation, but it might be not expected as well. 
a deep copy would be good.   Immutability is highly recommended.  This is Effective Java item 15 in the the 2nd edition.
@carl-mastrangelo Updated.
A rule of thumb is:

- Do not end with a dot if it's not a sentence, e.g. 'not a number'
- End with a dot if it's a sentence, e.g. 'It is not a number.'
- End with a dot if it's not a sentence but followed by a sentence, e.g. 'not a number. It has to be a number.;
@trustin Got it, thanks! 

Patch that resolved the issue:
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolverGroup.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolverGroup.java
index fb306082320..303c42a0988 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolverGroup.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolverGroup.java
@@ -42,23 +42,27 @@
 @UnstableApi
 public class DnsAddressResolverGroup extends AddressResolverGroup<InetSocketAddress> {
 
-    private final ChannelFactory<? extends DatagramChannel> channelFactory;
-    private final DnsServerAddressStreamProvider nameServerProvider;
+    private final DnsNameResolverBuilder dnsResolverBuilder;
 
     private final ConcurrentMap<String, Promise<InetAddress>> resolvesInProgress = newConcurrentHashMap();
     private final ConcurrentMap<String, Promise<List<InetAddress>>> resolveAllsInProgress = newConcurrentHashMap();
 
+    public DnsAddressResolverGroup(DnsNameResolverBuilder dnsResolverBuilder) {
+        this.dnsResolverBuilder = dnsResolverBuilder.copy();
+    }
+
     public DnsAddressResolverGroup(
             Class<? extends DatagramChannel> channelType,
             DnsServerAddressStreamProvider nameServerProvider) {
-        this(new ReflectiveChannelFactory<DatagramChannel>(channelType), nameServerProvider);
+        this(new DnsNameResolverBuilder());
+        dnsResolverBuilder.channelType(channelType).nameServerProvider(nameServerProvider);
     }
 
     public DnsAddressResolverGroup(
             ChannelFactory<? extends DatagramChannel> channelFactory,
             DnsServerAddressStreamProvider nameServerProvider) {
-        this.channelFactory = channelFactory;
-        this.nameServerProvider = nameServerProvider;
+        this(new DnsNameResolverBuilder());
+        dnsResolverBuilder.channelFactory(channelFactory).nameServerProvider(nameServerProvider);
     }
 
     @SuppressWarnings("deprecation")
@@ -70,7 +74,11 @@ protected final AddressResolver<InetSocketAddress> newResolver(EventExecutor exe
                     " (expected: " + StringUtil.simpleClassName(EventLoop.class));
         }
 
-        return newResolver((EventLoop) executor, channelFactory, nameServerProvider);
+        // we don't really need to pass channelFactory and nameServerProvider separately,
+        // but still keep this to ensure backward compatibility with (potentially) override methods
+        return newResolver((EventLoop) executor,
+                dnsResolverBuilder.channelFactory(),
+                dnsResolverBuilder.nameServerProvider());
     }
 
     /**
@@ -98,7 +106,9 @@ protected NameResolver<InetAddress> newNameResolver(EventLoop eventLoop,
                                                         ChannelFactory<? extends DatagramChannel> channelFactory,
                                                         DnsServerAddressStreamProvider nameServerProvider)
             throws Exception {
-        return new DnsNameResolverBuilder(eventLoop)
+        // once again, channelFactory and nameServerProvider are most probably set in builder already,
+        // but I do reassign them again to avoid corner cases with override methods
+        return dnsResolverBuilder.eventLoop(eventLoop)
                 .channelFactory(channelFactory)
                 .nameServerProvider(nameServerProvider)
                 .build();
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java
index 8ccf7c308f9..a5df3409dfe 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java
@@ -25,6 +25,7 @@
 import io.netty.util.internal.UnstableApi;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
 import static io.netty.resolver.dns.DnsServerAddressStreamProviders.platformDefault;
@@ -36,7 +37,7 @@
  */
 @UnstableApi
 public final class DnsNameResolverBuilder {
-    private final EventLoop eventLoop;
+    private EventLoop eventLoop;
     private ChannelFactory<? extends DatagramChannel> channelFactory;
     private DnsCache resolveCache;
     private DnsCache authoritativeDnsServerCache;
@@ -58,14 +59,35 @@ public final class DnsNameResolverBuilder {
     private int ndots = -1;
     private boolean decodeIdn = true;
 
+    /**
+     * Creates a new builder.
+     */
+    public DnsNameResolverBuilder() {
+    }
+
     /**
      * Creates a new builder.
      *
-     * @param eventLoop the {@link EventLoop} the {@link EventLoop} which will perform the communication with the DNS
+     * @param eventLoop the {@link EventLoop} which will perform the communication with the DNS
      * servers.
      */
     public DnsNameResolverBuilder(EventLoop eventLoop) {
+        eventLoop(eventLoop);
+    }
+
+    /**
+     * Sets the {@link EventLoop} which will perform the communication with the DNS servers.
+     *
+     * @param eventLoop the {@link EventLoop}
+     * @return {@code this}
+     */
+    public DnsNameResolverBuilder eventLoop(EventLoop eventLoop) {
         this.eventLoop = eventLoop;
+        return this;
+    }
+
+    protected ChannelFactory<? extends DatagramChannel> channelFactory() {
+        return this.channelFactory;
     }
 
     /**
@@ -274,6 +296,10 @@ public DnsNameResolverBuilder hostsFileEntriesResolver(HostsFileEntriesResolver
         return this;
     }
 
+    protected DnsServerAddressStreamProvider nameServerProvider() {
+        return this.dnsServerAddressStreamProvider;
+    }
+
     /**
      * Set the {@link DnsServerAddressStreamProvider} which is used to determine which DNS server is used to resolve
      * each hostname.
@@ -347,6 +373,10 @@ public DnsNameResolverBuilder decodeIdn(boolean decodeIdn) {
      * @return a {@link DnsNameResolver}
      */
     public DnsNameResolver build() {
+        if (eventLoop == null) {
+            throw new IllegalStateException("eventLoop should be specified to build a DnsNameResolver.");
+        }
+
         if (resolveCache != null && (minTtl != null || maxTtl != null || negativeTtl != null)) {
             throw new IllegalStateException("resolveCache and TTLs are mutually exclusive");
         }
@@ -377,4 +407,63 @@ public DnsNameResolver build() {
                 ndots,
                 decodeIdn);
     }
+
+    /**
+     * Creates a copy of this {@link DnsNameResolverBuilder}
+     *
+     * @return {@link DnsNameResolverBuilder}
+     */
+    public DnsNameResolverBuilder copy() {
+        DnsNameResolverBuilder copiedBuilder = new DnsNameResolverBuilder();
+
+        if (eventLoop != null) {
+            copiedBuilder.eventLoop(eventLoop);
+        }
+
+        if (channelFactory != null) {
+            copiedBuilder.channelFactory(channelFactory);
+        }
+
+        if (resolveCache != null) {
+            copiedBuilder.resolveCache(resolveCache);
+        }
+
+        if (maxTtl != null && minTtl != null) {
+            copiedBuilder.ttl(minTtl, maxTtl);
+        }
+
+        if (negativeTtl != null) {
+            copiedBuilder.negativeTtl(negativeTtl);
+        }
+
+        if (authoritativeDnsServerCache != null) {
+            copiedBuilder.authoritativeDnsServerCache(authoritativeDnsServerCache);
+        }
+
+        if (dnsQueryLifecycleObserverFactory != null) {
+            copiedBuilder.dnsQueryLifecycleObserverFactory(dnsQueryLifecycleObserverFactory);
+        }
+
+        copiedBuilder.queryTimeoutMillis(queryTimeoutMillis);
+        copiedBuilder.resolvedAddressTypes(resolvedAddressTypes);
+        copiedBuilder.recursionDesired(recursionDesired);
+        copiedBuilder.maxQueriesPerResolve(maxQueriesPerResolve);
+        copiedBuilder.traceEnabled(traceEnabled);
+        copiedBuilder.maxPayloadSize(maxPayloadSize);
+        copiedBuilder.optResourceEnabled(optResourceEnabled);
+        copiedBuilder.hostsFileEntriesResolver(hostsFileEntriesResolver);
+
+        if (dnsServerAddressStreamProvider != null) {
+            copiedBuilder.nameServerProvider(dnsServerAddressStreamProvider);
+        }
+
+        if (searchDomains != null) {
+            copiedBuilder.searchDomains(Arrays.asList(searchDomains));
+        }
+
+        copiedBuilder.ndots(ndots);
+        copiedBuilder.decodeIdn(decodeIdn);
+
+        return copiedBuilder;
+    }
 }
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/RoundRobinDnsAddressResolverGroup.java b/resolver-dns/src/main/java/io/netty/resolver/dns/RoundRobinDnsAddressResolverGroup.java
index 1fc072baa7c..68c14fec61f 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/RoundRobinDnsAddressResolverGroup.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/RoundRobinDnsAddressResolverGroup.java
@@ -36,6 +36,10 @@
 @UnstableApi
 public class RoundRobinDnsAddressResolverGroup extends DnsAddressResolverGroup {
 
+    public RoundRobinDnsAddressResolverGroup(DnsNameResolverBuilder dnsResolverBuilder) {
+        super(dnsResolverBuilder);
+    }
+
     public RoundRobinDnsAddressResolverGroup(
             Class<? extends DatagramChannel> channelType,
             DnsServerAddressStreamProvider nameServerProvider) {


Test patch:
diff --git a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
index 805121c7304..b2f5c3a9ee5 100644
--- a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
+++ b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
@@ -18,6 +18,7 @@
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.ByteBufHolder;
 import io.netty.channel.AddressedEnvelope;
+import io.netty.channel.ChannelFactory;
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.EventLoop;
 import io.netty.channel.EventLoopGroup;
@@ -1474,4 +1475,20 @@ public DnsCacheEntry cache(String hostname, DnsRecord[] additionals, Throwable c
         assertTrue(DnsNameResolver.isTransportOrTimeoutError(cause));
         resolver.close();
     }
+
+    @Test
+    public void testDnsNameResolverBuilderCopy() {
+        ChannelFactory<DatagramChannel> channelFactory =
+                new ReflectiveChannelFactory<DatagramChannel>(NioDatagramChannel.class);
+        DnsNameResolverBuilder builder = new DnsNameResolverBuilder(group.next())
+                .channelFactory(channelFactory);
+        DnsNameResolverBuilder copiedBuilder = builder.copy();
+
+        // change channel factory does not propagate to previously made copy
+        ChannelFactory<DatagramChannel> newChannelFactory =
+                new ReflectiveChannelFactory<DatagramChannel>(NioDatagramChannel.class);
+        builder.channelFactory(newChannelFactory);
+        assertEquals(channelFactory, copiedBuilder.channelFactory());
+        assertEquals(newChannelFactory, builder.channelFactory());
+    }
 }