Skip to content

Conversation

@JoshRosen
Copy link
Contributor

This patch fixes a compilation / build break in Spark's java8-tests and refactors their POM to simplify the build. See individual commit messages for more details.

</plugin>
<plugin>
<!-- disabled -->
<groupId>net.alchim31.maven</groupId>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK the idea behind this logic was to ensure that Java files would not be compiled by the Scala maven plugin, but disabling this has the side-effect of preventing this module's Scala test source file from being compiled and run in the Maven build.

Instead, I think it's okay to just modify the Scala Maven Plugin's javacArgs configuration.

@JoshRosen
Copy link
Contributor Author

I have updated the pull request builder Jenkins job to use a Java 8 JDK, so these tests should now be run.

It just occurred to me that I may want to configure the dev/sparktestsupport/modules.py file to make sure that these tests are automatically run if only a subset of tests would otherwise be run.

@SparkQA
Copy link

SparkQA commented Mar 30, 2016

Test build #54555 has finished for PR 12073 at commit 16b7823.

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

@vanzin
Copy link
Contributor

vanzin commented Mar 31, 2016

Out of curiosity, isn't all this work mostly throw away if we're really dropping jdk7 support? At that point you could just move the source files to core/src/test, and get rid of all the build-related code.

@JoshRosen
Copy link
Contributor Author

Yep, this would be irrelevant if we dropped JDK 7. However, I'm doing some other work which may have an impact on Java 8 lambda compatibility so I wanted to fix these tests to make my life a little easier.

@srowen
Copy link
Member

srowen commented Mar 31, 2016

+1. "Oops". Yeah we probably want to keep these tests in any event since they specifically test Java 8. Bigger question is just how far we want to go to update and streamline Java code and tests to use lambdas.

@JoshRosen
Copy link
Contributor Author

Any objections to these build changes or is this now good to merge?

@vanzin
Copy link
Contributor

vanzin commented Mar 31, 2016

Go ahead, it's a net improvement on the current state.

@JoshRosen
Copy link
Contributor Author

Merging now; thanks everyone.

@asfgit asfgit closed this in a7af6cd Mar 31, 2016
@JoshRosen JoshRosen deleted the fix-java8-tests branch March 31, 2016 20:57
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.

4 participants