You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/05/10 22:51:01 UTC
[GitHub] [kafka] badaiaqrandista opened a new pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
badaiaqrandista opened a new pull request #8644:
URL: https://github.com/apache/kafka/pull/8644
Modified ClientUtils#resolve to use "use_all_dns_ips" as default
Added "use_first_dns_ips" to get the old behaviour.
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r434168481
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,15 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
- return Collections.singletonList(addresses[0]);
+ switch (clientDnsLookup) {
+ case DEFAULT:
+ log.warn("Configuration 'client.dns.lookup' value 'default' is deprecated and will be removed in the future version.");
Review comment:
I'm adding the check and the warning to the constructor of KafkaProducer, KafkaConsumer and KafkaAdminClient because the log context does not exist in the Config classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r434171589
##########
File path: clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java
##########
@@ -42,9 +42,11 @@
+ "servers (you may want more than one, though, in case a server is down).";
public static final String CLIENT_DNS_LOOKUP_CONFIG = "client.dns.lookup";
- public static final String CLIENT_DNS_LOOKUP_DOC = "Controls how the client uses DNS lookups. If set to <code>use_all_dns_ips</code> then, when the lookup returns multiple IP addresses for a hostname,"
- + " they will all be attempted to connect to before failing the connection. Applies to both bootstrap and advertised servers."
- + " If the value is <code>resolve_canonical_bootstrap_servers_only</code> each entry will be resolved and expanded into a list of canonical names.";
+ public static final String CLIENT_DNS_LOOKUP_DOC = "Controls how the client uses DNS lookups."
+ + " If set to <code>use_all_dns_ips</code>, attempt to connect to all IP addresses returned by the lookup and use the first one that connects successfully."
+ + " If set to <code>default</code>, connect to the first IP address returned by the lookup, even if the lookup returns multiple IP addresses."
+ + " If set to <code>resolve_canonical_bootstrap_servers_only</code>, each entry will be resolved and expanded into a list of canonical names."
+ + " Note that <code>default</code> is deprecated and will be removed in future release.";
Review comment:
Sorry, I do not understand why you are referring to "advertised servers". Instead, I have changed the explanation to clarify that this only applies "if bootstrap hostname is an alias to multiple canonical names".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r432767466
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IP("use_first_dns_ip");
Review comment:
Ok. I have changed core code to use `ClientDnsLookup.USE_ALL_DNS_IPS` and leaving all clients tests except `ClientUtilsTest`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r432768002
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IPS("use_first_dns_ips");
Review comment:
@mimaison Based on @ijuma 's suggestion, I am not adding `use_first_dns_ip`. I am going to deprecate `default` as the value and make `use_all_dns_ips` as the default value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r434196900
##########
File path: clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java
##########
@@ -42,9 +42,11 @@
+ "servers (you may want more than one, though, in case a server is down).";
public static final String CLIENT_DNS_LOOKUP_CONFIG = "client.dns.lookup";
- public static final String CLIENT_DNS_LOOKUP_DOC = "Controls how the client uses DNS lookups. If set to <code>use_all_dns_ips</code> then, when the lookup returns multiple IP addresses for a hostname,"
- + " they will all be attempted to connect to before failing the connection. Applies to both bootstrap and advertised servers."
- + " If the value is <code>resolve_canonical_bootstrap_servers_only</code> each entry will be resolved and expanded into a list of canonical names.";
+ public static final String CLIENT_DNS_LOOKUP_DOC = "Controls how the client uses DNS lookups."
+ + " If set to <code>use_all_dns_ips</code>, attempt to connect to all IP addresses returned by the lookup and use the first one that connects successfully."
+ + " If set to <code>default</code>, connect to the first IP address returned by the lookup, even if the lookup returns multiple IP addresses."
+ + " If set to <code>resolve_canonical_bootstrap_servers_only</code>, each entry will be resolved and expanded into a list of canonical names."
+ + " Note that <code>default</code> is deprecated and will be removed in future release.";
Review comment:
Each of these configs have an impact on bootstrap and advertized servers. So, we should be clear on what they do for each case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r432865228
##########
File path: clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java
##########
@@ -42,9 +42,11 @@
+ "servers (you may want more than one, though, in case a server is down).";
public static final String CLIENT_DNS_LOOKUP_CONFIG = "client.dns.lookup";
- public static final String CLIENT_DNS_LOOKUP_DOC = "Controls how the client uses DNS lookups. If set to <code>use_all_dns_ips</code> then, when the lookup returns multiple IP addresses for a hostname,"
- + " they will all be attempted to connect to before failing the connection. Applies to both bootstrap and advertised servers."
- + " If the value is <code>resolve_canonical_bootstrap_servers_only</code> each entry will be resolved and expanded into a list of canonical names.";
+ public static final String CLIENT_DNS_LOOKUP_DOC = "Controls how the client uses DNS lookups."
+ + " If set to <code>use_all_dns_ips</code>, attempt to connect to all IP addresses returned by the lookup and use the first one that connects successfully."
+ + " If set to <code>default</code>, connect to the first IP address returned by the lookup, even if the lookup returns multiple IP addresses."
+ + " If set to <code>resolve_canonical_bootstrap_servers_only</code>, each entry will be resolved and expanded into a list of canonical names."
+ + " Note that <code>default</code> is deprecated and will be removed in future release.";
Review comment:
Can we clarify that `resolve_canonical_bootstrap_servers_only` applies to the boostrap urls only. For advertised servers, both `use_all_dns_ips` and `resolve_canonical_bootstrap_servers` behave the same.
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClientUtilsTest.java
##########
@@ -102,7 +102,7 @@ public void testResolveUnknownHostException() throws UnknownHostException {
@Test
public void testResolveDnsLookup() throws UnknownHostException {
- assertEquals(1, ClientUtils.resolve("localhost", ClientDnsLookup.DEFAULT).size());
+ assertEquals(1, ClientUtils.resolve("kafka.apache.org", ClientDnsLookup.DEFAULT).size());
Review comment:
Why do we need this change?
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,11 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
+ if (ClientDnsLookup.USE_FIRST_DNS_IP == clientDnsLookup) {
return Collections.singletonList(addresses[0]);
+ } else {
+ // ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup || ClientDnsLookup.DEFAULT == clientDnsLookup
+ return filterPreferredAddresses(addresses);
}
Review comment:
Can we remove the default since we handled all the cases?
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,15 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
- return Collections.singletonList(addresses[0]);
+ switch (clientDnsLookup) {
+ case DEFAULT:
+ log.warn("Configuration 'client.dns.lookup' value 'default' is deprecated and will be removed in the future version.");
+ return Collections.singletonList(addresses[0]);
+ case USE_ALL_DNS_IPS:
+ case RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY:
+ return filterPreferredAddresses(addresses);
Review comment:
Can we add a test that passes `RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY` showing that we now get multiple addresses instead of only the first one. That's one of the changes in this PR.
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,15 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
- return Collections.singletonList(addresses[0]);
+ switch (clientDnsLookup) {
+ case DEFAULT:
+ log.warn("Configuration 'client.dns.lookup' value 'default' is deprecated and will be removed in the future version.");
Review comment:
This will warn on every resolve. We should probably add the warning when we get the config value from `ConsumerConfig`, `ProducerConfig` and `AdminClientConfig`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#issuecomment-637879051
@ijuma Merged against trunk, fixed conflict in ConsumerConfig, and updated upgrade.html.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r432772760
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IPS("use_first_dns_ips");
Review comment:
I agree it makes sense to not introduce this new value. It's unfortunate the old value is called `default` but hopefully we'll remove it in the next major release.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431574455
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,11 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
+ if (ClientDnsLookup.USE_FIRST_DNS_IP == clientDnsLookup) {
return Collections.singletonList(addresses[0]);
+ } else {
+ // ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup || ClientDnsLookup.DEFAULT == clientDnsLookup
+ return filterPreferredAddresses(addresses);
}
Review comment:
@badaiaqrandista We should replace this if/else with a switch statement so that we are forced to handle every case. That is error prone if we add new elements to the enum. Also, can we please add tests?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma merged pull request #8644: KAFKA-9313: Set `use_all_dns_ips` as the new default for `client.dns.lookup` (KIP-602)
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #8644:
URL: https://github.com/apache/kafka/pull/8644
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431622505
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,11 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
+ if (ClientDnsLookup.USE_FIRST_DNS_IP == clientDnsLookup) {
return Collections.singletonList(addresses[0]);
+ } else {
+ // ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup || ClientDnsLookup.DEFAULT == clientDnsLookup
+ return filterPreferredAddresses(addresses);
}
Review comment:
Yes, you are correct. If `client.dns.lookup=RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY` , then it will resolves the hostname to IP address using the default behaviour.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#issuecomment-636247005
We should also add a note to upgrade.html.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431974209
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,11 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
+ if (ClientDnsLookup.USE_FIRST_DNS_IP == clientDnsLookup) {
return Collections.singletonList(addresses[0]);
+ } else {
+ // ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup || ClientDnsLookup.DEFAULT == clientDnsLookup
+ return filterPreferredAddresses(addresses);
}
Review comment:
I've changed this into switch/case and updated the KIP.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma edited a comment on pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma edited a comment on pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#issuecomment-637861009
Can you please rebase against trunk.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r434209688
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClientUtilsTest.java
##########
@@ -102,7 +102,7 @@ public void testResolveUnknownHostException() throws UnknownHostException {
@Test
public void testResolveDnsLookup() throws UnknownHostException {
- assertEquals(1, ClientUtils.resolve("localhost", ClientDnsLookup.DEFAULT).size());
+ assertEquals(1, ClientUtils.resolve("kafka.apache.org", ClientDnsLookup.DEFAULT).size());
Review comment:
Added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#issuecomment-637861009
Can you please rebase against master.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: Set `use_all_dns_ips` as the new default for `client.dns.lookup` (KIP-602)
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r435179495
##########
File path: clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java
##########
@@ -167,4 +176,13 @@
}
return rval;
}
+
+ public static void warnIfDeprecatedDnsLookupValue(AbstractConfig config) {
+ String clientDnsLookupValue = config.getString(CLIENT_DNS_LOOKUP_CONFIG);
+ if (clientDnsLookupValue.equals(ClientDnsLookup.DEFAULT.toString()))
+ log.warn("Configuration '{}' with value '{}' is deprecated and will be removed in " +
+ "future version. Please use '{}' or another non-deprecated value.",
+ CLIENT_DNS_LOOKUP_CONFIG, ClientDnsLookup.DEFAULT.toString(),
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r434227486
##########
File path: clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java
##########
@@ -42,9 +42,11 @@
+ "servers (you may want more than one, though, in case a server is down).";
public static final String CLIENT_DNS_LOOKUP_CONFIG = "client.dns.lookup";
- public static final String CLIENT_DNS_LOOKUP_DOC = "Controls how the client uses DNS lookups. If set to <code>use_all_dns_ips</code> then, when the lookup returns multiple IP addresses for a hostname,"
- + " they will all be attempted to connect to before failing the connection. Applies to both bootstrap and advertised servers."
- + " If the value is <code>resolve_canonical_bootstrap_servers_only</code> each entry will be resolved and expanded into a list of canonical names.";
+ public static final String CLIENT_DNS_LOOKUP_DOC = "Controls how the client uses DNS lookups."
+ + " If set to <code>use_all_dns_ips</code>, attempt to connect to all IP addresses returned by the lookup and use the first one that connects successfully."
+ + " If set to <code>default</code>, connect to the first IP address returned by the lookup, even if the lookup returns multiple IP addresses."
+ + " If set to <code>resolve_canonical_bootstrap_servers_only</code>, each entry will be resolved and expanded into a list of canonical names."
+ + " Note that <code>default</code> is deprecated and will be removed in future release.";
Review comment:
Config doc updated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r433548142
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClientUtilsTest.java
##########
@@ -102,7 +102,7 @@ public void testResolveUnknownHostException() throws UnknownHostException {
@Test
public void testResolveDnsLookup() throws UnknownHostException {
- assertEquals(1, ClientUtils.resolve("localhost", ClientDnsLookup.DEFAULT).size());
+ assertEquals(1, ClientUtils.resolve("kafka.apache.org", ClientDnsLookup.DEFAULT).size());
Review comment:
because `kafka.apache.org` resolves to 2 IP addresses. and I want to be sure that DEFAULT only returns 1 of them.
`localhost` resolves to 1 IP address.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431990981
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IP("use_first_dns_ip");
Review comment:
@badaiaqrandista I think you should change it in the non test code in this PR. For the test code, we can do it in a separate PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on pull request #8644: KAFKA-9313: Set `use_all_dns_ips` as the new default for `client.dns.lookup` (KIP-602)
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#issuecomment-638585687
@ijuma LGTM. Much clearer. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r434204693
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,15 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
- return Collections.singletonList(addresses[0]);
+ switch (clientDnsLookup) {
+ case DEFAULT:
+ log.warn("Configuration 'client.dns.lookup' value 'default' is deprecated and will be removed in the future version.");
Review comment:
Nevermind. I've added the warning to Config classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431576164
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IP("use_first_dns_ip");
Review comment:
We said this would be deprecated for removal in 3.0. Let's mention that in the documentation and add a warning if this is selected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431576336
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IP("use_first_dns_ip");
Review comment:
Thinking about it, maybe we should not add this at all. We could deprecate `default` and switch the default to `use_all_dns_ips`. Thoughts @badaiaqrandista and @rajinisivaram?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r430721875
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IPS("use_first_dns_ips");
Review comment:
In the KIP this field is called `use_first_dns_ip`, I think it was agreed in the discussion to remove the `s`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431574455
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,11 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
+ if (ClientDnsLookup.USE_FIRST_DNS_IP == clientDnsLookup) {
return Collections.singletonList(addresses[0]);
+ } else {
+ // ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup || ClientDnsLookup.DEFAULT == clientDnsLookup
+ return filterPreferredAddresses(addresses);
}
Review comment:
@badaiaqrandista We should replace this if/else with a switch statement so that we are forced to handle every case. That makes it easier to avoid issues if we add new elements to the enum. Also, can we please add tests?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r433543985
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IPS("use_first_dns_ips");
Review comment:
ok. resolving this conversation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r433544226
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,15 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
- return Collections.singletonList(addresses[0]);
+ switch (clientDnsLookup) {
+ case DEFAULT:
+ log.warn("Configuration 'client.dns.lookup' value 'default' is deprecated and will be removed in the future version.");
Review comment:
ok. will move the warn there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431621469
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IP("use_first_dns_ip");
Review comment:
I think that is a better idea, instead of introducing new value that would be removed soon.
`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
It is also used literally in a lot of test cases under clients.
I did not want to touch too many code in the first go. Should I change them all in this KIP or leave them until we remove `ClientDnsLookup.DEFAULT` in 3.0 ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #8644: KAFKA-9313: Set `use_all_dns_ips` as the new default for `client.dns.lookup` (KIP-602)
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r435128517
##########
File path: clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java
##########
@@ -167,4 +176,13 @@
}
return rval;
}
+
+ public static void warnIfDeprecatedDnsLookupValue(AbstractConfig config) {
+ String clientDnsLookupValue = config.getString(CLIENT_DNS_LOOKUP_CONFIG);
+ if (clientDnsLookupValue.equals(ClientDnsLookup.DEFAULT.toString()))
+ log.warn("Configuration '{}' with value '{}' is deprecated and will be removed in " +
+ "future version. Please use '{}' or another non-deprecated value.",
+ CLIENT_DNS_LOOKUP_CONFIG, ClientDnsLookup.DEFAULT.toString(),
Review comment:
We don't need to call `toString()`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431576017
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,11 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
+ if (ClientDnsLookup.USE_FIRST_DNS_IP == clientDnsLookup) {
return Collections.singletonList(addresses[0]);
+ } else {
+ // ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup || ClientDnsLookup.DEFAULT == clientDnsLookup
+ return filterPreferredAddresses(addresses);
}
Review comment:
Btw, it seems that we are changing the behavior of `RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY` as well, right? Before, we would only use the first DNS IP for that option in the non bootstrap path. It would be good to make that clear in the KIP.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r434204767
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,15 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
- return Collections.singletonList(addresses[0]);
+ switch (clientDnsLookup) {
+ case DEFAULT:
+ log.warn("Configuration 'client.dns.lookup' value 'default' is deprecated and will be removed in the future version.");
+ return Collections.singletonList(addresses[0]);
+ case USE_ALL_DNS_IPS:
+ case RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY:
+ return filterPreferredAddresses(addresses);
Review comment:
Added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431576336
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IP("use_first_dns_ip");
Review comment:
Thinking about it, maybe we should not add this at all. We could deprecate `default` and switch the config default to `use_all_dns_ips`. Thoughts @badaiaqrandista and @rajinisivaram?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #8644: KAFKA-9313: Set `use_all_dns_ips` as the new default for `client.dns.lookup` (KIP-602)
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#issuecomment-638622720
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #8644: KAFKA-9313: Set `use_all_dns_ips` as the new default for `client.dns.lookup` (KIP-602)
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#issuecomment-638845776
Thanks for the contribution! Merged to trunk and 2.6 branches.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] badaiaqrandista commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
badaiaqrandista commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r433547561
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientUtils.java
##########
@@ -108,10 +108,11 @@ public static ChannelBuilder createChannelBuilder(AbstractConfig config, Time ti
static List<InetAddress> resolve(String host, ClientDnsLookup clientDnsLookup) throws UnknownHostException {
InetAddress[] addresses = InetAddress.getAllByName(host);
- if (ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup) {
- return filterPreferredAddresses(addresses);
- } else {
+ if (ClientDnsLookup.USE_FIRST_DNS_IP == clientDnsLookup) {
return Collections.singletonList(addresses[0]);
+ } else {
+ // ClientDnsLookup.USE_ALL_DNS_IPS == clientDnsLookup || ClientDnsLookup.DEFAULT == clientDnsLookup
+ return filterPreferredAddresses(addresses);
}
Review comment:
ok. will remove the default.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8644: KAFKA-9313: [WIP] Make use_all_dns_ips as the default for client.dns.lookup
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8644:
URL: https://github.com/apache/kafka/pull/8644#discussion_r431571756
##########
File path: clients/src/main/java/org/apache/kafka/clients/ClientDnsLookup.java
##########
@@ -22,7 +22,8 @@
DEFAULT("default"),
USE_ALL_DNS_IPS("use_all_dns_ips"),
- RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only");
+ RESOLVE_CANONICAL_BOOTSTRAP_SERVERS_ONLY("resolve_canonical_bootstrap_servers_only"),
+ USE_FIRST_DNS_IPS("use_first_dns_ips");
Review comment:
I edited the PR to include this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org