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/09/16 21:14:45 UTC
[GitHub] [kafka] jolshan opened a new pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
jolshan opened a new pull request #9294:
URL: https://github.com/apache/kafka/pull/9294
ClusterConnectStatesTest and ClientUtilsTest were failing because they expected kafka.apache.org to resolve to 2 IP addresses. This updates the tests so they reflect that DNS resolves to 3 addresses.
----------------------------------------------------------------
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 #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-701683469
@mumrah I think 2.0 and higher would be a good target. We don't need separate PRs as long as the cherry pick is clean.
----------------------------------------------------------------
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] andrewegel commented on pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
andrewegel commented on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-701678141
> How far back should we backport?
Checking the AK -> CCS merge Job branches, the 2.3 through 2.5 branch unit tests are failing on the tests that this fixes.
----------------------------------------------------------------
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] jolshan commented on a change in pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#discussion_r491037285
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClusterConnectionStatesTest.java
##########
@@ -273,22 +273,21 @@ public void testMultipleIPsWithDefault() throws UnknownHostException {
@Test
public void testMultipleIPsWithUseAll() throws UnknownHostException {
- assertEquals(2, ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size());
+ assertTrue(ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size() > 1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr1 = connectionStates.currentAddress(nodeId1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr2 = connectionStates.currentAddress(nodeId1);
assertNotSame(addr1, addr2);
-
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr3 = connectionStates.currentAddress(nodeId1);
- assertSame(addr1, addr3);
Review comment:
I see. I changed it to show that the first three are different but I didn't include the check that it loops back to the first IP since that part seems the most fragile.
----------------------------------------------------------------
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] jolshan commented on pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-694319833
I also like @dajac's suggestion. I've updated the PR so that the tests assert more than 1 address as @mumrah said.
----------------------------------------------------------------
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] mumrah commented on a change in pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
mumrah commented on a change in pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#discussion_r490417685
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClusterConnectionStatesTest.java
##########
@@ -273,22 +273,21 @@ public void testMultipleIPsWithDefault() throws UnknownHostException {
@Test
public void testMultipleIPsWithUseAll() throws UnknownHostException {
- assertEquals(2, ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size());
+ assertTrue(ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size() > 1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr1 = connectionStates.currentAddress(nodeId1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr2 = connectionStates.currentAddress(nodeId1);
assertNotSame(addr1, addr2);
-
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr3 = connectionStates.currentAddress(nodeId1);
- assertSame(addr1, addr3);
Review comment:
Hmm, so previously (and also with this change) we are assuming that we will get different resolved IPs for each connection to apache.kafka.org? This seems to rely on round-robbin DNS resolution that we can't really control. I think these assertions will always be prone to failure unless we can control the DNS server like @dajac suggested.
I guess we can commit this to unblock the tests, but we should definitely prioritize a better fix.
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClientUtilsTest.java
##########
@@ -102,20 +102,20 @@ public void testResolveUnknownHostException() throws UnknownHostException {
@Test
public void testResolveDnsLookup() throws UnknownHostException {
- // Note that kafka.apache.org resolves to 2 IP addresses
+ // Note that kafka.apache.org resolves to at least 2 IP addresses
Review comment:
Comment here and below refer to 2 IP addresses still.
----------------------------------------------------------------
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] jolshan commented on a change in pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#discussion_r491037285
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClusterConnectionStatesTest.java
##########
@@ -273,22 +273,21 @@ public void testMultipleIPsWithDefault() throws UnknownHostException {
@Test
public void testMultipleIPsWithUseAll() throws UnknownHostException {
- assertEquals(2, ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size());
+ assertTrue(ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size() > 1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr1 = connectionStates.currentAddress(nodeId1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr2 = connectionStates.currentAddress(nodeId1);
assertNotSame(addr1, addr2);
-
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr3 = connectionStates.currentAddress(nodeId1);
- assertSame(addr1, addr3);
Review comment:
I see. I changed it to show that the first three are different but I didn't include the check that it loops back to the front since that part seems the most fragile.
----------------------------------------------------------------
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] mumrah commented on pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
mumrah commented on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-694490391
Thanks for the patch @jolshan!
----------------------------------------------------------------
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] mumrah commented on a change in pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
mumrah commented on a change in pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#discussion_r490861713
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClusterConnectionStatesTest.java
##########
@@ -273,22 +273,21 @@ public void testMultipleIPsWithDefault() throws UnknownHostException {
@Test
public void testMultipleIPsWithUseAll() throws UnknownHostException {
- assertEquals(2, ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size());
+ assertTrue(ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size() > 1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr1 = connectionStates.currentAddress(nodeId1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr2 = connectionStates.currentAddress(nodeId1);
assertNotSame(addr1, addr2);
-
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr3 = connectionStates.currentAddress(nodeId1);
- assertSame(addr1, addr3);
Review comment:
Thanks for clarifying, @ijuma. Seems like we could verify the round-robin behavior without relying on specific number of IPs.
----------------------------------------------------------------
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] mumrah commented on pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
mumrah commented on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-694216516
Agreed with @dajac's suggestion. Maybe for this change we could have the assertion as > 1 rather than equalling a specific 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] jolshan commented on a change in pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#discussion_r490423860
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClusterConnectionStatesTest.java
##########
@@ -273,22 +273,21 @@ public void testMultipleIPsWithDefault() throws UnknownHostException {
@Test
public void testMultipleIPsWithUseAll() throws UnknownHostException {
- assertEquals(2, ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size());
+ assertTrue(ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size() > 1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr1 = connectionStates.currentAddress(nodeId1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr2 = connectionStates.currentAddress(nodeId1);
assertNotSame(addr1, addr2);
-
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr3 = connectionStates.currentAddress(nodeId1);
- assertSame(addr1, addr3);
Review comment:
Yeah. I was wondering if we should even keep this test in the current form, and I'm hoping that a DNS we can control would resolve this issue.
----------------------------------------------------------------
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] dajac commented on pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-694131412
Netty used a in-memory DNS server for such tests: https://github.com/netty/netty/blob/master/resolver-dns/src/test/java/io/netty/resolver/dns/TestDnsServer.java. I suppose that we could do something similar in AK as well.
What about merging this one and filing a JIRA to improve this in the future? All these flaky tests due to this are quite annoying so we could already fix them. What do you think?
It would be great if we could backport it to previous versions as well.
----------------------------------------------------------------
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] mumrah edited a comment on pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
mumrah edited a comment on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-694805060
2.6 backport applied cleanly, kicked off a build here https://ci-builds.apache.org/job/Kafka/job/kafka-2.6-jdk8/15/
~2.5 had a conflict, and one test fails for me locally. I'll look into it along with other branches this evening~
Edit: looks like the behavior of ClientUtils#resolve changed in 2.6 (84020bfc13) so the fix will be slightly different for older versions. How far back should we backport? I see jenkins jobs back to 2.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] jolshan commented on a change in pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#discussion_r490461933
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClientUtilsTest.java
##########
@@ -102,20 +102,20 @@ public void testResolveUnknownHostException() throws UnknownHostException {
@Test
public void testResolveDnsLookup() throws UnknownHostException {
- // Note that kafka.apache.org resolves to 2 IP addresses
+ // Note that kafka.apache.org resolves to at least 2 IP addresses
Review comment:
Are you saying to change it back? I thought it was fair to say "at least 2" to reflect we are asserting greater than 1.
----------------------------------------------------------------
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] jolshan commented on pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-694325685
Also created the issue https://issues.apache.org/jira/browse/KAFKA-10496
----------------------------------------------------------------
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 #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#discussion_r490699517
##########
File path: clients/src/test/java/org/apache/kafka/clients/ClusterConnectionStatesTest.java
##########
@@ -273,22 +273,21 @@ public void testMultipleIPsWithDefault() throws UnknownHostException {
@Test
public void testMultipleIPsWithUseAll() throws UnknownHostException {
- assertEquals(2, ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size());
+ assertTrue(ClientUtils.resolve(hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS).size() > 1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr1 = connectionStates.currentAddress(nodeId1);
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr2 = connectionStates.currentAddress(nodeId1);
assertNotSame(addr1, addr2);
-
connectionStates.connecting(nodeId1, time.milliseconds(), hostTwoIps, ClientDnsLookup.USE_ALL_DNS_IPS);
InetAddress addr3 = connectionStates.currentAddress(nodeId1);
- assertSame(addr1, addr3);
Review comment:
The test is verifying the round robin that _our_ code does, not the DNS server. We should ensure we still have that coverage in the meantime.
----------------------------------------------------------------
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 #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-694647346
Have we cherry picked this to older branches? It seems like they will be broken otherwise.
----------------------------------------------------------------
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] jolshan commented on pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-693727406
@abbccdda I agree that they are fragile. Could you give a little more detail about checking the patterns? I'm not sure if I follow.
----------------------------------------------------------------
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] mumrah merged pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
mumrah merged pull request #9294:
URL: https://github.com/apache/kafka/pull/9294
----------------------------------------------------------------
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] andrewegel removed a comment on pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
andrewegel removed a comment on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-701678141
> How far back should we backport?
Checking the AK -> CCS merge Job branches, the 2.3 through 2.5 branch unit tests are failing on the tests that this fixes.
----------------------------------------------------------------
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] mumrah commented on pull request #9294: MINOR: Fix now that kafka.apache.org resolves to 3 IP addresses
Posted by GitBox <gi...@apache.org>.
mumrah commented on pull request #9294:
URL: https://github.com/apache/kafka/pull/9294#issuecomment-694805060
2.6 backport applied cleanly, kicked off a build here https://ci-builds.apache.org/job/Kafka/job/kafka-2.6-jdk8/15/
2.5 had a conflict, and one test fails for me locally. I'll look into it along with other branches this evening
----------------------------------------------------------------
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