Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-14134] [core] [test-maven] Change the package name used for shading classes. #11941

Closed
wants to merge 4 commits into from
Closed

[SPARK-14134] [core] [test-maven] Change the package name used for shading classes. #11941

wants to merge 4 commits into from

Conversation

vanzin
Copy link
Contributor

@vanzin vanzin commented Mar 24, 2016

The current package name uses a dash, which is a little weird but seemed
to work. That is, until a new test tried to mock a class that references
one of those shaded types, and then things started failing.

Most changes are just noise to fix the logging configs.

For reference, SPARK-8815 also raised this issue, although at the time it
did not cause any issues in Spark, so it was not addressed.

[SPARK-14134] [core] Change the package name used for shading classes.
9ecbafb 
The current package name uses a dash, which is a little weird but seemed
to work. That is, until a new test tried to mock a class that references
one of those shaded types, and then things started failing.

Most changes are just noise to fix the logging configs.
srowen
srowen reviewed Mar 24, 2016
View reviewed changes
pom.xml
@@ -178,6 +178,9 @@
<test.java.home>${java.home}</test.java.home>
<test.exclude.tags></test.exclude.tags>

<!-- Package to use when relocating shaded classes. -->
<spark.shade.packageName>org.spark_project</spark.shade.packageName>
Copy link
Member

Choose a reason for hiding this comment

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

For total avoidance of trouble, is org.sparkproject safer? or are you sure underscores are more OK than hyphens? BTW you could consider this PR as a resolution for SPARK-8815 too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Underscores are ok (they're not an operator like - is).

@vanzin vanzin changed the title [SPARK-14134] [core] Change the package name used for shading classes. [SPARK-14134] [core] [test-maven] Change the package name used for shading classes. Mar 24, 2016
@vanzin
Copy link
Contributor Author

vanzin commented Mar 24, 2016

retest this please

@SparkQA
Copy link

SparkQA commented Mar 24, 2016

Test build #54073 has finished for PR 11941 at commit 9ecbafb.

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

@vanzin
Copy link
Contributor Author

vanzin commented Mar 24, 2016

Weird error, seems unrelated. retest this please.

@SparkQA
Copy link

SparkQA commented Mar 24, 2016

Test build #54067 has finished for PR 11941 at commit 9ecbafb.

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

@vanzin
Copy link
Contributor Author

vanzin commented Mar 24, 2016

hmm different test failure that also looks unrelated. retest this please

@SparkQA
Copy link

SparkQA commented Mar 25, 2016

Test build #54083 has finished for PR 11941 at commit 9ecbafb.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor Author

vanzin commented Mar 25, 2016

sql test was stuck. retest this please.

vanzin pushed a commit to vanzin/spark that referenced this pull request Mar 25, 2016
Fix for SPARK-14134 (apache#11941).
3ae4e02
@SparkQA
Copy link

SparkQA commented Mar 25, 2016

Test build #54099 has finished for PR 11941 at commit 9ecbafb.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor Author

vanzin commented Mar 25, 2016

wtf. retest this please.

@SparkQA
Copy link

SparkQA commented Mar 25, 2016

Test build #54132 has finished for PR 11941 at commit 9ecbafb.

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

@SparkQA
Copy link

SparkQA commented Mar 25, 2016

Test build #54137 has finished for PR 11941 at commit 9ecbafb.

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

@vanzin
Copy link
Contributor Author

vanzin commented Mar 25, 2016

this is the same flaky test that's failing in other places with maven... let me see if it's easy to fix.

@zsxwing
Copy link
Member

zsxwing commented Mar 25, 2016

@vanzin I fixed it in #11895

@zsxwing
Copy link
Member

zsxwing commented Mar 25, 2016

However, #11895 is blocked by #11940. It can not pass tests :(

@vanzin
Copy link
Contributor Author

vanzin commented Mar 25, 2016

Cool, guess I'll wait for those, thanks.

@vanzin
Copy link
Contributor Author

vanzin commented Mar 26, 2016

retest this please

@SparkQA
Copy link

SparkQA commented Mar 26, 2016

Test build #54242 has finished for PR 11941 at commit 9ecbafb.

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

vanzin pushed a commit to vanzin/spark that referenced this pull request Mar 28, 2016
Revert "Fix for SPARK-14134 (apache#11941)."
eb828ce 
This reverts commit 3ae4e02.
@zsxwing
Copy link
Member

zsxwing commented Mar 28, 2016

You also need to update the flume's log4j files as it's moved back now.

@SparkQA
Copy link

SparkQA commented Mar 28, 2016

Test build #54332 has finished for PR 11941 at commit 81af47b.

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

@vanzin
Copy link
Contributor Author

vanzin commented Mar 28, 2016

I'm gonna close this for now since #11796 is passing without this patch... no idea how.

@vanzin vanzin closed this Mar 28, 2016
@vanzin vanzin reopened this Apr 6, 2016
@vanzin
Copy link
Contributor Author

vanzin commented Apr 6, 2016

Reopened since I'm running into this problem locally, and the fix makes things more correct anyway.

@SparkQA
Copy link

SparkQA commented Apr 6, 2016

Test build #55138 has finished for PR 11941 at commit 21568f9.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor Author

vanzin commented Apr 6, 2016

retest this please

@SparkQA
Copy link

SparkQA commented Apr 6, 2016

Test build #55141 has finished for PR 11941 at commit 21568f9.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor Author

vanzin commented Apr 6, 2016

@JoshRosen these tests are not happy with the build jdk; are you still in the process of fixing things?

@JoshRosen
Copy link
Contributor

Argh, I forgot to do the $PATH prepending in the PRB... forget that those jobs' confs are managed via a different system than the rest of the Jenkins confs. Applying a manual conf. change to those jobs now.

@JoshRosen
Copy link
Contributor

Jenkins, retest this please.

@JoshRosen JoshRosen mentioned this pull request Apr 6, 2016
Closed
@SparkQA
Copy link

SparkQA commented Apr 6, 2016

Test build #55148 has finished for PR 11941 at commit 21568f9.

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

@vanzin
Copy link
Contributor Author

vanzin commented Apr 7, 2016

Since this was reviewed / tested before, I'll merge it in the morning unless someone objects (or beats me to it).

@JoshRosen
Copy link
Contributor

LGTM, so I'm going to merge this now into 2.0. Thanks Marcelo!

@asfgit asfgit closed this in 21d5ca1 Apr 7, 2016
@vanzin vanzin deleted the SPARK-14134 branch April 25, 2019 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vanzin @SparkQA @zsxwing @JoshRosen @srowen