You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Badai Aqrandista <ba...@confluent.io> on 2020/04/28 00:46:10 UTC
[DISCUSS] KIP-602 - Change default value for client.dns.lookup
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
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Badai Aqrandista <ba...@confluent.io>.
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 <is...@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 <ra...@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 <is...@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 <
> > rajinisivaram@gmail.com
> > > >
> > > >> wrote:
> > > >>
> > > >> > Deprecating for removal in 3.0 sounds good.
> > > >> >
> > > >> > On Wed, May 20, 2020 at 3:33 PM Ismael Juma <is...@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 <
> > > rajinisivaram@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 <
> > > >> badai@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 <
> > > >> badai@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 <
> > > >> > badai@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 <
> > > >> ismael@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 <
> > > >> > > badai@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
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Ismael Juma <is...@juma.me.uk>.
+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 <ra...@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 <is...@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 <
> rajinisivaram@gmail.com
> > >
> > >> wrote:
> > >>
> > >> > Deprecating for removal in 3.0 sounds good.
> > >> >
> > >> > On Wed, May 20, 2020 at 3:33 PM Ismael Juma <is...@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 <
> > rajinisivaram@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 <
> > >> badai@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 <
> > >> badai@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 <
> > >> > badai@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 <
> > >> ismael@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 <
> > >> > > badai@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
> >
>
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Rajini Sivaram <ra...@gmail.com>.
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 <is...@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 <rajinisivaram@gmail.com
> >
> >> wrote:
> >>
> >> > Deprecating for removal in 3.0 sounds good.
> >> >
> >> > On Wed, May 20, 2020 at 3:33 PM Ismael Juma <is...@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 <
> rajinisivaram@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 <
> >> badai@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 <
> >> badai@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 <
> >> > badai@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 <
> >> ismael@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 <
> >> > > badai@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
>
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Badai Aqrandista <ba...@confluent.io>.
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 <is...@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 <ra...@gmail.com>
>> wrote:
>>
>> > Deprecating for removal in 3.0 sounds good.
>> >
>> > On Wed, May 20, 2020 at 3:33 PM Ismael Juma <is...@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 <rajinisivaram@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 <
>> badai@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 <
>> badai@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 <
>> > badai@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 <
>> ismael@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 <
>> > > badai@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
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Badai Aqrandista <ba...@confluent.io>.
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 <is...@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 <ra...@gmail.com>
> wrote:
>
> > Deprecating for removal in 3.0 sounds good.
> >
> > On Wed, May 20, 2020 at 3:33 PM Ismael Juma <is...@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 <ra...@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 <badai@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 <
> badai@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 <
> > badai@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 <ismael@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 <
> > > badai@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
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Ismael Juma <is...@juma.me.uk>.
Badai, would you like to start a vote on this KIP?
Ismael
On Wed, May 20, 2020 at 7:45 AM Rajini Sivaram <ra...@gmail.com>
wrote:
> Deprecating for removal in 3.0 sounds good.
>
> On Wed, May 20, 2020 at 3:33 PM Ismael Juma <is...@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 <ra...@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 <badai@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 <
> badai@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 <is...@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 <
> > badai@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
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Rajini Sivaram <ra...@gmail.com>.
Deprecating for removal in 3.0 sounds good.
On Wed, May 20, 2020 at 3:33 PM Ismael Juma <is...@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 <ra...@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 <badai@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 <is...@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 <
> badai@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
> > >
> >
>
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Ismael Juma <is...@juma.me.uk>.
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 <ra...@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 <is...@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 <badai@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
> >
>
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Rajini Sivaram <ra...@gmail.com>.
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 <is...@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
>
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Badai Aqrandista <ba...@confluent.io>.
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 <is...@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
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Badai Aqrandista <ba...@confluent.io>.
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 <is...@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
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Badai Aqrandista <ba...@confluent.io>.
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 <is...@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
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
Posted by Ismael Juma <is...@juma.me.uk>.
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
>