[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
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 use_all_dns_ips 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 resolve_canonical_bootstrap_servers_only 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 use_all_dns_ips, attempt to connect to all IP addresses returned by the lookup and use the first one that connects successfully." + + " If set to default, connect to the first IP address returned by the lookup, even if the lookup returns multiple IP addresses." + + " If set to resolve_canonical_bootstrap_servers_only, each entry will be resolved and expanded into a list of canonical names." + + " Note that default 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: us...@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
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:
us...@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
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 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:
us...@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
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 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:
us...@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
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 use_all_dns_ips 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 resolve_canonical_bootstrap_servers_only 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 use_all_dns_ips, attempt to connect to all IP addresses returned by the lookup and use the first one that connects successfully." + + " If set to default, connect to the first IP address returned by the lookup, even if the lookup returns multiple IP addresses." + + " If set to resolve_canonical_bootstrap_servers_only, each entry will be resolved and expanded into a list of canonical names." + + " Note that default 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: us...@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
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 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:
us...@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
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:
us...@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
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 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:
us...@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
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 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:
us...@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
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:
us...@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
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:
us...@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
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:
us...@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
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 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:
us...@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
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 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:
us...@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
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:
us...@infra.apache.org
