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
