Ismael/Rajini I have updated the KIP to reflect the deprecation of "default" value and not adding "use_first_dns_ip". Also I have updated the PR to reflect this change, including changing all references of ClientDnsLookup.DEFAULT to ClientDnsLookup.USE_ALL_DNS_IPS in core code and clients test code.
Please let me know what you think. Thanks Badai On Fri, May 29, 2020 at 3:46 AM Ismael Juma <ism...@juma.me.uk> wrote: > +1. I think we should remove this config in AK 3.0, but in the meantime, we > can log a warning if people explicitly set the value to `default`. I think > this would be pretty rare. > > Ismael > > On Thu, May 28, 2020 at 10:25 AM Rajini Sivaram <rajinisiva...@gmail.com> > wrote: > > > It is a bit confusing to have a `default` value that is not the default > and > > in hindsight it wasn't a good choice of name. But agree that changing the > > config default and avoiding the temporary `use_first_dns_ip` option makes > > sense. > > > > Regards, > > > > Rajini > > > > > > On Thu, May 28, 2020 at 4:49 PM Badai Aqrandista <ba...@confluent.io> > > wrote: > > > > > Ismael/Rajini > > > > > > I have put some comments in the PR in response to Ismael's. > > > > > > I have some questions about Ismael's suggestion to not add > > > "use_first_dns_ip" at all and instead just deprecate "default". > > > > > > The PR would be much cleaner if we just deprecate "default" as you > > > suggested. But we will need to update some core code. And I will need > to > > > update the KIP to reflect this. > > > > > > ClientDnsLookup.DEFAULT is used in few places in core (server): > > > > > > > > > > > > https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/KafkaServer.scala#L520 > > > > > > > > > https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/ReplicaFetcherBlockingSend.scala#L92 > > > > > > > > > https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/controller/ControllerChannelManager.scala#L156 > > > > > > > > > https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/coordinator/transaction/TransactionMarkerChannelManager.scala#L82 > > > > > > And a couple of tools: > > > > > > > > > > > > https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala#L482 > > > > > > > > > https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/admin/BrokerApiVersionsCommand.scala#L299 > > > > > > And some tests. > > > > > > What do you think? > > > > > > Thanks > > > Badai > > > > > > On Fri, May 22, 2020 at 6:45 PM Badai Aqrandista <ba...@confluent.io> > > > wrote: > > > > > > > Voting thread has been posted. > > > > > > > > KIP-602 page has been updated with suggestions from Rajini. > > > > > > > > Thanks > > > > Badai > > > > > > > > On Fri, May 22, 2020 at 6:00 AM Ismael Juma <ism...@juma.me.uk> > wrote: > > > > > > > >> Badai, would you like to start a vote on this KIP? > > > >> > > > >> Ismael > > > >> > > > >> On Wed, May 20, 2020 at 7:45 AM Rajini Sivaram < > > rajinisiva...@gmail.com > > > > > > > >> wrote: > > > >> > > > >> > Deprecating for removal in 3.0 sounds good. > > > >> > > > > >> > On Wed, May 20, 2020 at 3:33 PM Ismael Juma <ism...@juma.me.uk> > > > wrote: > > > >> > > > > >> > > Is there any reason to use "use_first_dns_ip"? Should we remove > it > > > >> > > completely? Or at least deprecate it for removal in 3.0? > > > >> > > > > > >> > > Ismael > > > >> > > > > > >> > > > > > >> > > On Wed, May 20, 2020, 1:39 AM Rajini Sivaram < > > > rajinisiva...@gmail.com > > > >> > > > > >> > > wrote: > > > >> > > > > > >> > > > 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 > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > > > > -- > > > > Thanks, > > > > Badai > > > > > > > > > > > > > > -- > > > Thanks, > > > Badai > > > > > > -- Thanks, Badai
