Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/17#issuecomment-36436518
  
    Hey @ScrapCodes - this is really cool! It's great that we will get this 
into Spark 1.0 so that we can support Java 8 lambdas moving forward. I have 
some high level suggestions focused on the build and documentation:
    
    1. Would you mind writing a "Upgrading from pre 1.0 versions of Spark" 
section in the Java programming guide. It should go over the backwards 
incompatible stuff and explain what users will need to change.
    
    2. Would you mind adding breif README file in the java8-tests directory 
that explains what it is? From what I can tell these need to be isolated in a 
separate directory since we don't require JDK 8 to build Spark. It would be 
good to explain that, because it looks pretty ugly having that top-level 
directory sitting there and people might get confused.
    
    3. When running the tests in Maven, I noticed this puts all of the output 
to the console, and actually it didn't show the test summaries as they were 
running. This will blow up the output on Jenkins and makes it hard to see 
what's going on. I think we've configured the other Maven tests to output to a 
file and show summaries, it would be good if this matched how those work. 
    
    4. I found it hard to get SBT to use the correct Java version without 
setting Java 8 as my system default. I was able to fix that with the following 
changes - mind adding these?
    A. Change the sbt/sbt script so it respects JAVA_HOME similar to 
./spark-class. Right now it just uses the "java" which is not easy to switch 
around between versions.
    B. In SparkBuild I pass through JAVA_HOME.
    ```
         javacOptions := Seq("-target", JAVAC_JVM_VERSION, "-source", 
JAVAC_JVM_VERSION),
    +    // Pass through user defined JAVA_HOME if set
    +    javaHome := Option(System.getenv("JAVA_HOME")).map(h => new File(h)),
    
    ```
    
    5. Would you mind adding a warning to `dev/run-tests` script if the Java 
version is less than 1.8? Unfortunately I think this will mean again checking 
for JAVA_HOME. We should print a warning at the top saying that Java 8 tests 
will not run if the JDK version is < 1.8.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to