[SPARK-6440][CORE]Handle IPv6 addresses properly when constructing URI

[nyaapa]

No description provided.

[srowen]

Tell me more about what this fixes?

[nyaapa]

It's all about finding our external IP address: old logic was to use external ipv4 || 127.0.0.1.
That transformes it to ipv4 || ipv6 || 127.0.0.1

Whenewer we don't have any ipv4 it's better to use ipv6 than 127.0.0.1

[srowen]

OK, though I don't think that was the thrust of this JIRA, which was simply to format IPv6 addresses correctly. I think the reasoning makes some sense though.

Nits: can you filterNot(addr => addr.isLinkLocalAddress || addr.isLoopbackAddress)
and if (addresses.notEmpty) {? and find(_.isInstanceOf[Inet4Address])? Just questions of style in this block.

It might be worth a comment to the effect of your reply above here.

[nyaapa]

Ok, i agree with that. Should i make additional commit or change existing one?
*nonEmpty

[srowen]

Just push more commits to this branch

[srowen]

ok to test

[SparkQA]

Test build #29936 has finished for PR 5424 at commit 84763d7.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[nyaapa]

Use getHostAddress instead of getHostName ( as it was before ), because of getHostName may return additional information like 95.108.174.236-red.dhcp.yndx.net.

[SparkQA]

Test build #29940 has started for PR 5424 at commit 2098081.

[shaneknapp]

jenkins, test this please

[SparkQA]

Test build #29943 has finished for PR 5424 at commit 2098081.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[nyaapa]

Seems like jenkins was killed again.

[shaneknapp]

whoops, i missed retriggering this build. :)

jenkins, test this please

[srowen]

Jenkins, retest this please.

[SparkQA]

Test build #29962 has finished for PR 5424 at commit 2098081.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[JoshRosen]

Jenkins, retest this please.

[SparkQA]

Test build #29974 has finished for PR 5424 at commit 2098081.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[srowen]

I think I've looked at 10 different versions of 2 PRs for this JIRA now, and all seem to be changing or adding something slightly different than what's essential here. Let me describe what I think needs to be done and why and we can settle that, then implement.

The code needs a local IP address in many cases. That's localIpAddress. (Separately, your update to find an IPv6 address sounds OK.)

localIpAddressHostname and the getAddressHostName() method actually look pointless. They make a String into an InetAddress and then ask for the exact same text representation back as a String. Personally I'd make localIpAddress an InetAddress, as you've done. I'd remove localIpAddressHostname and replace with localHostName() where a String is needed.

... because then customHostname is consistently applied, right? I think that must be the intent?

But then, localIpAddress itself doesn't have much value. It is always used for its string rep anyway. So, localHostName() as well? it's the only thing that needs to be non-private.

Back to the original issue: where localHostName() would then be used in a URI, it can be run through the utility method in InetAddresses. I don't think that we should instead have more methods in Utils just for adding a call to that method.

Finally, there are instances in SparkSQLEnv, KinesisReceiver and TestUtils that use InetAddress.getLocalHost directly. These should also use localHostName().

Maybe there's a reason that doesn't 100% work but it seems a lot less messy than the 3-5 ways code is reasoning about the local IP address string now.

What say, or, should I try it in my own PR?

[nyaapa]

Ok, i'll try to:

• replace localIpAddressHostname in Utils.localHostName with localIpAddress.getHostAddress
• replace localIpAddressHostname usage with localHostName
• remove localIpAddressHostname and getAddressHostName
• make localIpAddress private
• fix SparkSQLEnv, KinesisReceiver and TestUtils

today/tomorrow.

Back to the original issue: where localHostName() would then be used in a URI, it can be run through the utility method in InetAddresses.
I don't think that we should instead have more methods in Utils just for adding a call to that method.

I didn't find any method in InetAddresses like escapeHostname(String) => String. InetAddresses.toUriString(InetAddress.getByName(Utils.localHostName())), but this looks pointless like localIpAddressHostname = InetAddress.getByName(localIpAddress).getHostName.
And there we have two alternatives:

1. remove localIpAddressURI variable and change def localHostURI() = customHostname.getOrElse(InetAddresses.toUriString(localIpAddress))
2. ask everyone to use InetAddresses.toUriString(InetAddress.getByName(Utils.localHostName()))

imho additional method with is better. Also i don't like conversion from String to InetAddress and even worse is that we got String from InetAddress.

[srowen]

InetAddresses.toUriString accepts a String and returns an InetAddress, so it does not need its input changed. Yes you have to call getHostName on the output.

[nyaapa]

InetAddresses.toUriString is (InetAddress) => String, maybe you meant forUriString(String) => InetAddress or forString(String) => InetAddress ?
for forUriString

scala> val ipv6 = InetAddress.getByAddress(NetworkInterface.getNetworkInterfaces.toList.apply(0).getInetAddresses.toList.apply(0).getAddress).getHostAddress
ipv6: String = fe80:0:0:0:3623:87ff:fe6a:423c

scala> InetAddresses.forUriString(ipv6)
java.lang.IllegalArgumentException: Not a valid URI IP literal: 'fe80:0:0:0:3623:87ff:fe6a:423c'
  at com.google.common.net.InetAddresses.forUriString(InetAddresses.java:501)
  ... 33 elided

And for forString i still need InetAddresses.toUriString

scala> val ipv6 = InetAddress.getByAddress(NetworkInterface.getNetworkInterfaces.toList.apply(0).getInetAddresses.toList.apply(0).getAddress).getHostAddress
ipv6: String = fe80:0:0:0:3623:87ff:fe6a:423c

scala> InetAddresses.forString(ipv6).getHostName
res35: String = fe80:0:0:0:3623:87ff:fe6a:423c

scala> InetAddresses.forString(ipv6).getHostAddress
res36: String = fe80:0:0:0:3623:87ff:fe6a:423c

scala> InetAddresses.toUriString(InetAddresses.forString(ipv6))
res37: String = [fe80::3623:87ff:fe6a:423c]

[srowen]

Oh gosh yeah looking at this the wrong way around of course. Aren't there just two places where the URI logic is needed? I think calling InetAddresses.toUriString(InetAddress.getByName(Utils.localHostName())) in two places isn't so bad. A new method is OK too if you rprefer; I suppose ... localHostNameForURI() or something to make the relation clear? Then we have just two methods for accessing 'the local host name' with a clear role.

[nyaapa]

Ok, localHostNameForURI sounds reasonable.
About network/common/src/test/java/org/apache/spark/network/TestUtils.java: there is just only one method -- getLocalHost() => String, i can delete the whole class and use Utils.localHostname instead, but maybe using scala from java it's not a good practice?

[srowen]

I'd just leave TestUtils for purposes here, but can make it call through to Utils.

[SparkQA]

Test build #30091 has finished for PR 5424 at commit 6b717aa.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[srowen]

LGTM. This simplifies and standardizes the local host name logic across the code, fixes the original problem of IPv6 addresses in URIs, chooses a better local address in some IPv6 cases. Of course, making things consistent means slight behavior changes in some cases -- for example, customHostName now applies in cases it didn't before, but this actually seems more correct. I've eyeballed every change and it looks logical, and the tests pass.