[SPARK-32103][CORE] Support IPv6 host/port in core module #28931

PavithraRamachandran · 2020-06-26T07:23:01Z

What changes were proposed in this pull request?

In IPv6 scenario, the current logic to split hostname and port is not correct.

Why are the changes needed?

to support IPV6 deployment scenario

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT and IPV6 spark deployment with yarn

dongjoon-hyun · 2020-06-27T02:53:22Z

ok to test

dongjoon-hyun · 2020-06-27T02:54:16Z

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnRMClient.scala

    val prefix = WebAppUtils.getHttpSchemePrefix(conf)
    val proxies = WebAppUtils.getProxyHostsAndPortsForAmFilter(conf)
-    val hosts = proxies.asScala.map(_.split(":").head)
+    val hosts = proxies.asScala.map(proxy => proxy.splitAt(proxy.lastIndexOf(":"))._1)


Hi, @PavithraRamachandran . If possible, could you check the code base if this is the last instance or not?

I have identified few more places where IPV6 is not handled, i shall update my PR soon

SparkQA · 2020-06-27T03:20:41Z

Test build #124558 has finished for PR 28931 at commit 3e2eb3a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

In general, new implementation seems to introduce a new assumption and less robust as a result. For example, when we get host name only without port due to YARN API change, it will fail. It may be unlikely, but we had better keep the robustness here if there is no other reason @PavithraRamachandran .

scala> val proxies = Seq("host")
proxies: Seq[String] = List(host)

scala> proxies.map(_.split(":").head)
res16: Seq[String] = List(host)

scala> proxies.map(proxy => proxy.splitAt(proxy.lastIndexOf(":"))._1)
res17: Seq[String] = List("")

dongjoon-hyun · 2020-06-30T18:28:03Z

Gentle ping, @PavithraRamachandran .

PavithraRamachandran · 2020-07-01T10:42:54Z

@dongjoon-hyun thank you for identifying the missed scenario, i shall handle it and update the PR

SparkQA · 2020-07-02T19:59:22Z

Test build #124919 has finished for PR 28931 at commit 98a737b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-02T20:28:33Z

Test build #124920 has finished for PR 28931 at commit becec4e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-07-02T20:31:19Z

Retest this please.

PavithraRamachandran · 2020-07-03T13:42:53Z

@dongjoon-hyun i am getting 502 proxy error when i try to access the CI to check for failures. Could u help me?

dongjoon-hyun · 2020-07-03T17:23:00Z

GitHub Action CI is okay, but UCB AmbLab Jenkins CI seems to be completely down now. In this case, there is no way for us to do. Let's wait for a while.

SparkQA · 2020-07-05T20:37:06Z

Test build #124942 has finished for PR 28931 at commit becec4e.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-07-08T15:19:38Z

Thank you for updating, @PavithraRamachandran .

dongjoon-hyun · 2020-07-08T15:19:48Z

Retest this please

dongjoon-hyun · 2020-07-08T15:20:10Z

cc @dbtsai

core/src/main/scala/org/apache/spark/util/Utils.scala

dongjoon-hyun · 2020-07-08T15:28:44Z

core/src/test/scala/org/apache/spark/util/UtilsSuite.scala

+    Utils.checkHost("0.0.0.0")
+    intercept[AssertionError] {
+      Utils.checkHost("0.0.0.0:0")
+    }


In this case, it would be great if we check the error message to prevent future regression.

dongjoon-hyun · 2020-07-08T15:30:22Z

core/src/test/scala/org/apache/spark/util/UtilsSuite.scala

    assert(Utils.buildLocationMetadata(paths, 25) == "[path0, path1, path2, path3]")
  }
+
+  test("checkHosts support IPV6 and IPV4") {


checkHosts -> checkHost

support -> supports

IPV6 and IPV4 -> both IPv4 and IPv6?

dongjoon-hyun · 2020-07-08T15:30:55Z

core/src/test/scala/org/apache/spark/util/UtilsSuite.scala

+    assert(hostnamePort._1.equals("0.0.0.0"))
+    assert(hostnamePort._2 === 0)
+
+


nit. repeated blank lines.

SparkQA · 2020-07-08T22:32:11Z

Test build #125379 has finished for PR 28931 at commit 4c9cb2e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

…reaks the logic when we use ipv6 deployment

SparkQA · 2020-07-10T19:32:55Z

Test build #125609 has finished for PR 28931 at commit 768e7c4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

+1, LGTM. Thank you, @PavithraRamachandran .
Merged to master for Apache Spark 3.1.0. Definitely, this is a good step toward IPv6 support.

PavithraRamachandran · 2020-07-13T05:02:51Z

thank u for merging @dongjoon-hyun

gatorsmile · 2020-07-15T02:08:42Z

Is it the only blocker for IPV6 support?

dongjoon-hyun · 2020-07-15T05:25:05Z

Hi, @gatorsmile . Technically, this only handles host/port parsing inside core module. I'm sure that this is a meaningful step inside Spark. However, we didn't test anything on IPv6. Like what we did for JDK11, I expect lots of hurdle both inside and outside Spark.

### What changes were proposed in this pull request? In IPv6 scenario, the current logic to split hostname and port is not correct. ### Why are the changes needed? to support IPV6 deployment scenario ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT and IPV6 spark deployment with yarn Closes #28931 from PavithraRamachandran/ipv6_issue. Authored-by: Pavithraramachandran <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

probot-autolabeler bot added the YARN label Jun 26, 2020

PavithraRamachandran changed the title ~~[SPARK-32103] [yarn] Handle host/port split in IPV6 in YarnRMClient~~ [SPARK-32103] [yarn] Handle host/port split in IPV6 in YarnRMClient Jun 26, 2020

PavithraRamachandran changed the title ~~[SPARK-32103] [yarn] Handle host/port split in IPV6 in YarnRMClient~~ [SPARK-32103] Handle host/port split in IPV6 in YarnRMClient Jun 26, 2020

dongjoon-hyun reviewed Jun 27, 2020

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-32103] Handle host/port split in IPV6 in YarnRMClient~~ [SPARK-32103][YARN] Handle IPv6 host/port split in YarnRMClient Jun 28, 2020

dongjoon-hyun requested changes Jun 28, 2020

View reviewed changes

PavithraRamachandran force-pushed the ipv6_issue branch from 3e2eb3a to 98a737b Compare July 2, 2020 16:41

probot-autolabeler bot added the CORE label Jul 2, 2020

PavithraRamachandran force-pushed the ipv6_issue branch from 98a737b to becec4e Compare July 2, 2020 16:48

PavithraRamachandran force-pushed the ipv6_issue branch from becec4e to 4c9cb2e Compare July 8, 2020 14:43

dongjoon-hyun reviewed Jul 8, 2020

View reviewed changes

core/src/main/scala/org/apache/spark/util/Utils.scala Show resolved Hide resolved

dongjoon-hyun reviewed Jul 8, 2020

View reviewed changes

core/src/main/scala/org/apache/spark/util/Utils.scala Show resolved Hide resolved

dongjoon-hyun reviewed Jul 8, 2020

View reviewed changes

[SPARK-32103] spark is using ":" to het the host and port, but this b…

768e7c4

…reaks the logic when we use ipv6 deployment

PavithraRamachandran force-pushed the ipv6_issue branch from 4c9cb2e to 768e7c4 Compare July 10, 2020 12:52

dongjoon-hyun changed the title ~~[SPARK-32103][YARN] Handle IPv6 host/port split in YarnRMClient~~ [SPARK-32103][CORE] Support IPv6 host/port in core module Jul 10, 2020

dongjoon-hyun approved these changes Jul 10, 2020

View reviewed changes

dongjoon-hyun closed this in d7d5bdf Jul 10, 2020

		assert(hostnamePort._1.equals("0.0.0.0"))
		assert(hostnamePort._2 === 0)

[SPARK-32103][CORE] Support IPv6 host/port in core module #28931

[SPARK-32103][CORE] Support IPv6 host/port in core module #28931

Uh oh!

Conversation

PavithraRamachandran commented Jun 26, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

dongjoon-hyun commented Jun 27, 2020

Uh oh!

dongjoon-hyun Jun 27, 2020

Choose a reason for hiding this comment

Uh oh!

PavithraRamachandran Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 27, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jun 30, 2020

Uh oh!

PavithraRamachandran commented Jul 1, 2020

Uh oh!

SparkQA commented Jul 2, 2020

Uh oh!

SparkQA commented Jul 2, 2020

Uh oh!

dongjoon-hyun commented Jul 2, 2020

Uh oh!

PavithraRamachandran commented Jul 3, 2020

Uh oh!

dongjoon-hyun commented Jul 3, 2020

Uh oh!

SparkQA commented Jul 5, 2020

Uh oh!

dongjoon-hyun commented Jul 8, 2020

Uh oh!

dongjoon-hyun commented Jul 8, 2020

Uh oh!

dongjoon-hyun commented Jul 8, 2020

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 8, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

PavithraRamachandran commented Jul 13, 2020

Uh oh!

gatorsmile commented Jul 15, 2020

Uh oh!

dongjoon-hyun commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PavithraRamachandran Jul 1, 2020 •

edited

Loading

dongjoon-hyun commented Jul 15, 2020 •

edited

Loading