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-17421] [DOCS] Documenting the current treatment of MAVEN_OPTS. #15005

Closed
wants to merge 6 commits into from
Closed

[SPARK-17421] [DOCS] Documenting the current treatment of MAVEN_OPTS. #15005

wants to merge 6 commits into from

Conversation

frreiss
Copy link
Contributor

@frreiss frreiss commented Sep 7, 2016

What changes were proposed in this pull request?

Modified the documentation to clarify that build/mvn and pom.xml always add Java 7-specific parameters to MAVEN_OPTS, and that developers can safely ignore warnings about -XX:MaxPermSize that may result from compiling or running tests with Java 8.

How was this patch tested?

Rebuilt HTML documentation, made sure that building-spark.html displays correctly in a browser.

@frreiss frreiss changed the title [SPARK-17421] Documenting the current treatment of MAVEN_OPTS. [SPARK-17421] [DOCS] Documenting the current treatment of MAVEN_OPTS. Sep 7, 2016
srowen
srowen reviewed Sep 9, 2016
View reviewed changes
docs/building-spark.md
@@ -16,24 +16,27 @@ Building Spark using Maven requires Maven 3.3.9 or newer and Java 7+.

### Setting up Maven's Memory Usage

You'll need to configure Maven to use more memory than usual by setting `MAVEN_OPTS`. We recommend the following settings:
If you are compiling with Java 7, you'll need to configure Maven to use more memory than usual by setting `MAVEN_OPTS`:
Copy link
Member

Choose a reason for hiding this comment

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

This is still needed for Java 8, just not MaxPermSize.

Copy link
Member

Choose a reason for hiding this comment

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

I actually think the ReservedCodeCacheSize advice is defunct. I don't use it and don't have problems. But, maybe not worth worrying about here. It has nothing to do with the warnings discussed in this section though.

Anyway, might I finally suggest that, because Java 8 is more the default now and soon to be required, that this advice basically say:

Add Xmx and XX:ReservedCodeCacheSize to MAVEN_OPTS
Or else you might see this error
If you're on Java 7 by the way, also add this MaxPermSize param like so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving out -XX:ReservedCodeCacheSize caused a warning in my tests:
OpenJDK 64-Bit Server VM warning: CodeCache is full. Compiler has been disabled. OpenJDK 64-Bit Server VM warning: Try increasing the code cache size using -XX:ReservedCodeCacheSize=
Left that option in and reworded the beginning of this section as you suggested.

@frreiss
Copy link
Contributor Author

frreiss commented Sep 9, 2016

Sure, I'll redo that part so that includes two sets of recommended options. Note that docs in the Spark 2.0.0 release say that these options aren't necessary for Java 8.

srowen
srowen reviewed Sep 12, 2016
View reviewed changes
docs/building-spark.md

**Note:**

* For Java 8 and above this step is not required.
* If using `build/mvn` with no `MAVEN_OPTS` set, the script will automate this for you.
* This step is not needed for Java 8.
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is now wrong and can be removed.

@frreiss
Copy link
Contributor Author

frreiss commented Sep 14, 2016

Quick update: I'm running a series of test builds with various parameters to determine what parts of MAVEN_OPTS are currently necessary on different versions of Java. Will report back in a few days and update this PR.

@srowen
Copy link
Member

srowen commented Sep 20, 2016

@frreiss are you in a position to finish this one?

@frreiss
Copy link
Contributor Author

frreiss commented Sep 20, 2016

I've about narrowed down the options that work for OpenJDK 7 and 8 on Mac and Linux. Working on IBM Java on Linux. I can have an update in by EOD today.
BTW, one thing that's been slowing me down is that my local copies get "corrupted" after a while. Some of the tests in SparkSubmitSuite stop working until I delete and re-clone my local copy of Spark. I suspect there may be a race condition involving some directory that doesn't get cleaned by the "clean" target.

@frreiss
Copy link
Contributor Author

frreiss commented Sep 21, 2016

Summary of testing:

  • On Java 8, the build fails intermittently with OOM when -Xmx2g is omitted
  • The -XX:ReservedCodeCacheSize=512m argument prevents warnings on both Java 7 and OpenJDK 8
  • -XX:ReservedCodeCacheSize=512m has no effect on IBM Java 8

srowen
srowen reviewed Sep 21, 2016
View reviewed changes
docs/building-spark.md

**Note:**

* For Java 8 and above this step is not required.
* If using `build/mvn` with no `MAVEN_OPTS` set, the script will automate this for you.
* If using `build/mvn` with no `MAVEN_OPTS` set, the script will automatically add the above options for Java 7 to the `MAVEN_OPTS` environment variable.
Copy link
Member

Choose a reason for hiding this comment

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

It actually adds them regardless of the Java version, it seems. Which is fine. The rest looks good. If you get to that change, great, otherwise I can probably safely make the change on merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I meant to say was that build/mvn adds all three options, but I can see how the statement here can interpreted differently. Checking in an edited version.

@SparkQA
Copy link

SparkQA commented Sep 21, 2016

Test build #3284 has finished for PR 15005 at commit 774a101.

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

@SparkQA
Copy link

SparkQA commented Sep 22, 2016

Test build #3286 has finished for PR 15005 at commit 53a09cd.

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

asfgit pushed a commit that referenced this pull request Sep 22, 2016
@frreiss @srowen
[SPARK-17421][DOCS] Documenting the current treatment of MAVEN_OPTS.
b25a8e6 
## What changes were proposed in this pull request?

Modified the documentation to clarify that `build/mvn` and `pom.xml` always add Java 7-specific parameters to `MAVEN_OPTS`, and that developers can safely ignore warnings about `-XX:MaxPermSize` that may result from compiling or running tests with Java 8.

## How was this patch tested?

Rebuilt HTML documentation, made sure that building-spark.html displays correctly in a browser.

Author: frreiss <frreiss@us.ibm.com>

Closes #15005 from frreiss/fred-17421a.

(cherry picked from commit 646f383)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@srowen
Copy link
Member

srowen commented Sep 22, 2016

Merged to master/2.0

@asfgit asfgit closed this in 646f383 Sep 22, 2016
@frreiss
Copy link
Contributor Author

frreiss commented Sep 23, 2016

Thanks @srowen for all the thoughtful comments! It's great to see committers spending time to help improve the build experience for new developers.

@peter-toth peter-toth mentioned this pull request Jun 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srowen srowen srowen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frreiss @srowen @SparkQA