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

Reply via email to