[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
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 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 > 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 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 >>> 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
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 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 > 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 > > 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 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 > >>> 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
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 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 > 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 > > 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 > > > 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 > 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 > > > >>> 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
Deprecating for removal in 3.0 sounds good. On Wed, May 20, 2020 at 3:33 PM Ismael Juma 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 > 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 > > 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 > > > 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 > > > > > 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 > > 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
Badai, would you like to start a vote on this KIP? Ismael On Wed, May 20, 2020 at 7:45 AM Rajini Sivaram wrote: > Deprecating for removal in 3.0 sounds good. > > On Wed, May 20, 2020 at 3:33 PM Ismael Juma 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 > > 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 > > > 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 > > > > > 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 > > > 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
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 wrote: > Badai, would you like to start a vote on this KIP? > > Ismael > > On Wed, May 20, 2020 at 7:45 AM Rajini Sivaram > wrote: > > > Deprecating for removal in 3.0 sounds good. > > > > On Wed, May 20, 2020 at 3:33 PM Ismael Juma 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 > > > 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 > > > > > 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 > > > > > 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 ap
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
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 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 wrote: > >> Badai, would you like to start a vote on this KIP? >> >> Ismael >> >> On Wed, May 20, 2020 at 7:45 AM Rajini Sivaram >> wrote: >> >> > Deprecating for removal in 3.0 sounds good. >> > >> > On Wed, May 20, 2020 at 3:33 PM Ismael Juma 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 > > >> > > 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 >> >
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
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 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 > 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 wrote: > > > >> Badai, would you like to start a vote on this KIP? > >> > >> Ismael > >> > >> On Wed, May 20, 2020 at 7:45 AM Rajini Sivaram > > >> wrote: > >> > >> > Deprecating for removal in 3.0 sounds good. > >> > > >> > On Wed, May 20, 2020 at 3:33 PM Ismael Juma > 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 > >> > > > > >> Clust
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
+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 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 > 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 > > 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 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 > > 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 > > >> > > i
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
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 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 > 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 > > 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 > > > 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 > 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 > > > 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 cr
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
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 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 >
Re: [DISCUSS] KIP-602 - Change default value for client.dns.lookup
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 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 > 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
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 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 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 >> 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