Hi Badai, Thanks for the KIP, sounds like a useful change. Perhaps we should call the new option `use_first_dns_ip` (not `_ips` since it refers to one). We should also mention in the KIP that only one type of address (ipv4 or ipv6, based on the first one) will be used - that is the current behaviour for `use_all_dns_ips`. Since we are changing `default` to be exactly the same as `use_all_dns_ips`, it will be good to mention that explicitly under Public Interfaces.
Regards, Rajini On Mon, May 18, 2020 at 1:44 AM Badai Aqrandista <ba...@confluent.io> wrote: > Ismael > > What do you think of the PR and the explanation regarding the issue raised > in KIP-235? > > Should I go ahead and build a proper PR? > > Thanks > Badai > > On Mon, May 11, 2020 at 8:53 AM Badai Aqrandista <ba...@confluent.io> > wrote: > > > Ismael > > > > PR created: https://github.com/apache/kafka/pull/8644/files > > > > Also, as this is my first PR, please let me know if I missed anything. > > > > Thanks > > Badai > > > > On Mon, May 11, 2020 at 8:19 AM Badai Aqrandista <ba...@confluent.io> > > wrote: > > > >> Ismael > >> > >> Thank you for responding. > >> > >> KIP-235 modified ClientUtils#parseAndValidateAddresses [1] to resolve an > >> address alias (i.e. bootstrap server) into multiple addresses. This is > why > >> it would break SSL hostname verification when the bootstrap server is > an IP > >> address, i.e. it will resolve the IP address to an FQDN and use that > FQDN > >> in the SSL handshake. > >> > >> However, what I am proposing is to modify ClientUtils#resolve [2], which > >> is only used in ClusterConnectionStates#currentAddress [3], to get the > >> resolved InetAddress of the address to connect to. And > >> ClusterConnectionStates#currentAddress is only used by > >> NetworkClient#initiateConnect [4] to create InetSocketAddress to > establish > >> the socket connection to the broker. > >> > >> Therefore, as far as I know, this change will not affect higher level > >> protocol like SSL or SASL. > >> > >> PR coming after this. > >> > >> Thanks > >> Badai > >> > >> [1] > >> > https://github.com/apache/kafka/blob/2.5.0/clients/src/main/java/org/apache/kafka/clients/ClientUtils.java#L51 > >> [2] > >> > https://github.com/apache/kafka/blob/2.5.0/clients/src/main/java/org/apache/kafka/clients/ClientUtils.java#L111 > >> [3] > >> > https://github.com/apache/kafka/blob/2.5.0/clients/src/main/java/org/apache/kafka/clients/ClusterConnectionStates.java#L403 > >> [4] > >> > https://github.com/apache/kafka/blob/2.5.0/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java#L955 > >> > >> On Sun, May 10, 2020 at 10:18 AM Ismael Juma <ism...@juma.me.uk> wrote: > >> > >>> Hi Badai, > >>> > >>> I think this is a good change. Can you please address the issues raised > >>> by KIP-235? That was the reason why we did not do it previously. > >>> > >>> Ismael > >>> > >>> On Mon, Apr 27, 2020 at 5:46 PM Badai Aqrandista <ba...@confluent.io> > >>> wrote: > >>> > >>>> Hi everyone > >>>> > >>>> I have opened this KIP to have client.dns.lookup default value changed > >>>> to > >>>> "use_all_dns_ips". > >>>> > >>>> > >>>> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-602%3A+Change+default+value+for+client.dns.lookup > >>>> > >>>> Feedback appreciated. > >>>> > >>>> PS: I'm new here so please let me know if I miss anything. > >>>> > >>>> -- > >>>> Thanks, > >>>> Badai > >>>> > >>> > >> > >> -- > >> Thanks, > >> Badai > >> > >> > > > > -- > > Thanks, > > Badai > > > > > > -- > Thanks, > Badai >
