[SPARK-20708][CORE] Make `addExclusionRules` up-to-date by dongjoon-hyun · Pull Request #17947 · apache/spark

dongjoon-hyun · 2017-05-11T08:25:07Z

What changes were proposed in this pull request?

Since SPARK-9263, resolveMavenCoordinates ignores Spark and Spark's dependencies by using addExclusionRules. This PR aims to make addExclusionRules up-to-date to neglect correctly because it fails to neglect some components like the following.

mllib (correct)

$ bin/spark-shell --packages org.apache.spark:spark-mllib_2.11:2.1.1
...
---------------------------------------------------------------------
|                  |            modules            ||   artifacts   |
|       conf       | number| search|dwnlded|evicted|| number|dwnlded|
---------------------------------------------------------------------
|      default     |   0   |   0   |   0   |   0   ||   0   |   0   |
---------------------------------------------------------------------

mllib-local (wrong)

$ bin/spark-shell --packages org.apache.spark:spark-mllib-local_2.11:2.1.1
...
---------------------------------------------------------------------
|                  |            modules            ||   artifacts   |
|       conf       | number| search|dwnlded|evicted|| number|dwnlded|
---------------------------------------------------------------------
|      default     |   15  |   2   |   2   |   0   ||   15  |   2   |
---------------------------------------------------------------------

How was this patch tested?

Pass the Jenkins with a updated test case.

SparkQA · 2017-05-11T11:19:49Z

Test build #76793 has finished for PR 17947 at commit 6ac403a.

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

dongjoon-hyun · 2017-05-12T16:04:38Z

Hi, @srowen .
Could you review this PR about --packages?

dongjoon-hyun · 2017-05-19T17:16:32Z

Retest this please.

SparkQA · 2017-05-19T19:36:06Z

Test build #77098 has finished for PR 17947 at commit 6ac403a.

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

dongjoon-hyun · 2017-05-19T20:17:52Z

Retest this please.

SparkQA · 2017-05-19T23:11:25Z

Test build #77103 has finished for PR 17947 at commit 6ac403a.

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

jiangxb1987 · 2017-05-26T21:26:15Z

cc @brkyvz to verify the change.

dongjoon-hyun · 2017-05-27T02:44:48Z

Thank you, @jiangxb1987 .

dongjoon-hyun · 2017-05-30T21:29:10Z

Could you review this please, @brkyvz ?

brkyvz · 2017-05-30T21:36:27Z


    val coordinates =
-      components.map(comp => s"org.apache.spark:spark-${comp}2.10:1.2.0").mkString(",") +
+      components.map(comp => s"org.apache.spark:spark-${comp}2.10:2.1.1").mkString(",") +


maybe also change Scala version too, since we will remove 2.10 soon

Sure! Thank you for review.

brkyvz · 2017-05-30T21:41:21Z

  test("neglects Spark and Spark's dependencies") {
-    val components = Seq("catalyst_", "core_", "graphx_", "hive_", "mllib_", "repl_",
-      "sql_", "streaming_", "yarn_", "network-common_", "network-shuffle_", "network-yarn_")
+    val components = Seq("catalyst_", "core_", "graphx_", "hive_", "hive-thriftserver_",


since the hive, mesos stuff is not in the assembly jar by default, I don't think we need to exclude those

same for yarn

I see. I'll remove hive, hive-thriftserver, mesos, and yarn.

Maybe, network-yarn_, too.

brkyvz · 2017-05-30T21:44:43Z

We shouldn't neglect dependencies that are not in the assembly jar by default. Otherwise thanks for doing this!

brkyvz · 2017-05-30T21:58:03Z

    // spark-streaming_2.1x and spark-streaming-kafka-0-8-assembly_2.1x
-    val components = Seq("catalyst_", "core_", "graphx_", "hive_", "mllib_", "repl_",
-      "sql_", "streaming_", "yarn_", "network-common_", "network-shuffle_", "network-yarn_")
+    val components = Seq("catalyst_", "core_", "graphx_", "launcher_", "mllib_", "mllib-local_",


can we also make this a static variable and re-use in tests?

otherwise it's hard to keep both in sync

brkyvz · 2017-05-30T21:58:22Z


    val coordinates =
-      components.map(comp => s"org.apache.spark:spark-${comp}2.10:1.2.0").mkString(",") +
+      components.map(comp => s"org.apache.spark:spark-${comp}2.11:2.1.1").mkString(",") +


SparkQA · 2017-05-31T00:36:46Z

Test build #77554 has finished for PR 17947 at commit bfa5238.

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

SparkQA · 2017-05-31T01:20:52Z

Test build #77560 has finished for PR 17947 at commit d562ea2.

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

dongjoon-hyun · 2017-05-31T17:04:12Z

Hi, @brkyvz .
Could you review this again?

brkyvz · 2017-06-01T00:10:46Z

+  // We need to specify each component explicitly, otherwise we miss spark-streaming-kafka-0-8 and
+  // other spark-streaming utility components. Underscore is there to differentiate between
+  // spark-streaming_2.1x and spark-streaming-kafka-0-8-assembly_2.1x
+  val components = Seq("catalyst_", "core_", "graphx_", "launcher_", "mllib_", "mllib-local_",


since it's in global scope now can we rename it to something more meaningful?
IVY_DEFAULT_EXCLUDES

Also add to the docs, since it has lost its context from the method and the docs now

dongjoon-hyun · 2017-06-01T00:32:13Z

@brkyvz . Sure. I renamed it into IVY_DEFAULT_EXCLUDES and add more documents about that.

SparkQA · 2017-06-01T00:35:06Z

Test build #77614 has finished for PR 17947 at commit 8b184e7.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-06-01T03:58:05Z

Test build #77615 has finished for PR 17947 at commit 2cf5400.

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

brkyvz · 2017-06-01T05:38:38Z

Thanks @dongjoon-hyun LGTM. Merging to master

dongjoon-hyun · 2017-06-01T14:10:21Z

Thank you so much, @brkyvz .

[SPARK-20708][CORE] Make addExclusionRules up-to-date

6ac403a

brkyvz reviewed May 30, 2017

View reviewed changes

Address comments.

bfa5238

brkyvz reviewed May 30, 2017

View reviewed changes

Make as a static variable of SparkSubmitUtils object.

d562ea2

brkyvz reviewed Jun 1, 2017

View reviewed changes

Rename the variable and add documents.

8b184e7

fix scalastyle.

2cf5400

asfgit closed this in 34661d8 Jun 1, 2017

dongjoon-hyun deleted the SPARK-20708 branch January 7, 2019 07:04

Conversation

dongjoon-hyun commented May 11, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 11, 2017

Uh oh!

dongjoon-hyun commented May 12, 2017

Uh oh!

dongjoon-hyun commented May 19, 2017

Uh oh!

SparkQA commented May 19, 2017

Uh oh!

dongjoon-hyun commented May 19, 2017

Uh oh!

SparkQA commented May 19, 2017

Uh oh!

jiangxb1987 commented May 26, 2017

Uh oh!

dongjoon-hyun commented May 27, 2017

Uh oh!

dongjoon-hyun commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brkyvz commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 31, 2017

Uh oh!

SparkQA commented May 31, 2017

Uh oh!

dongjoon-hyun commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jun 1, 2017

Uh oh!

SparkQA commented Jun 1, 2017

Uh oh!

SparkQA commented Jun 1, 2017

Uh oh!

brkyvz commented Jun 1, 2017

Uh oh!

dongjoon-hyun commented Jun 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants