Skip to content

Conversation

@pwendell
Copy link
Contributor

This patch piggy-back's on @vanzin's work to simplify the Guava shading,
and adds Jetty as a shaded library in Spark. Other than adding Jetty,
it consilidates the 's into the root pom. I found it was
a bit easier to follow that way, since you don't need to look into
child pom's to find out specific artifact sets included in shading.

@pwendell
Copy link
Contributor Author

Jenkins, test this please.

@SparkQA
Copy link

SparkQA commented Jan 28, 2015

Test build #26242 has started for PR 4252 at commit 62429c9.

  • This patch merges cleanly.

@pwendell
Copy link
Contributor Author

@vanzin - would you be able to take a look?

@vanzin
Copy link
Contributor

vanzin commented Jan 28, 2015

Hi @pwendell ,

Did you check that jetty classes only show up in spark-core's jar? I see a jetty dependency in streaming/pom.xml, so I'm afraid that would cause jetty to show up there too.

That would mean doing the guava thing: declare dependencies as provided in the root pom, and promote them to compile in core/pom.xml.

@pwendell
Copy link
Contributor Author

Hey @vanzin - here the jetty classes show up in both core and streaming. Is that a problem?

@vanzin
Copy link
Contributor

vanzin commented Jan 28, 2015

It's not a real problem other than being wasteful, but it should be easy to fix it.

@SparkQA
Copy link

SparkQA commented Jan 28, 2015

Test build #26242 has finished for PR 4252 at commit 62429c9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class SparkListenerExecutorAdded(time: Long, executorId: String, executorInfo: ExecutorInfo)
    • case class SparkListenerExecutorRemoved(time: Long, executorId: String, reason: String)
    • class BlockMatrix(

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26242/
Test PASSed.

@pwendell
Copy link
Contributor Author

Changed approach to only shade inside of core jar per @vanzin's suggestion.

pom.xml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: guava is promoted in network-common, so maybe rephrase this a little.

@SparkQA
Copy link

SparkQA commented Jan 28, 2015

Test build #26250 has started for PR 4252 at commit ac2e214.

  • This patch merges cleanly.

@vanzin
Copy link
Contributor

vanzin commented Jan 28, 2015

Latest patch LGTM. Thanks for the extra cleanup too!

@vanzin
Copy link
Contributor

vanzin commented Jan 28, 2015

Hey @pwendell,

Just remembered that there's an extra bit that needs to be taken care of when shading. core/pom.xml has a copy-dependencies task to copy certain artifacts into the build directory. That is used when running with SPARK_PREPEND_CLASSES since the raw compiled classes do not go through the relocation process.

That needs to be extended to Jetty too. Also I should probably take a look if that is still working for Guava since that's not a direct dependency anymore in core/...

@SparkQA
Copy link

SparkQA commented Jan 28, 2015

Test build #26250 has finished for PR 4252 at commit ac2e214.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26250/
Test PASSed.

This patch piggy-back's on @vanzin's work to simplify the Guava shading,
and adds Jetty as a shaded library in Spark. Other than adding Jetty,
it consilidates the <artifactSet>'s into the root pom. I found it was
a bit easier to follow that way, since you don't need to look into
child pom's to find out specific artifact sets included in shading.
@pwendell
Copy link
Contributor Author

@vanzin I've addressed your additional comments. Mind taking a look?

@vanzin
Copy link
Contributor

vanzin commented Jan 29, 2015

LGTM. Ship it!

@SparkQA
Copy link

SparkQA commented Jan 29, 2015

Test build #26337 has started for PR 4252 at commit 19f0710.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 29, 2015

Test build #26337 has finished for PR 4252 at commit 19f0710.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26337/
Test PASSed.

@vanzin
Copy link
Contributor

vanzin commented Jan 29, 2015

BTW I think @mccheah was interested in this originally.

@mccheah
Copy link
Contributor

mccheah commented Jan 29, 2015

@ash211

Thanks for doing this. My colleagues and I will test this appropriately when we find the bandwidth!

@asfgit asfgit closed this in f240fe3 Jan 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants