Skip to content

[#460] improvement: Exit on OutOfMemoryError - #1390

Merged
zuston merged 5 commits into
apache:masterfrom
lifeSo:feature_oom_460
Dec 21, 2023
Merged

[#460] improvement: Exit on OutOfMemoryError#1390
zuston merged 5 commits into
apache:masterfrom
lifeSo:feature_oom_460

Conversation

@lifeSo

@lifeSo lifeSo commented Dec 20, 2023

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Exit on OutOfMemoryError

Why are the changes needed?

Fix: #460

Does this PR introduce any user-facing change?

No.

How was this patch tested?

@zuston zuston changed the title [460][Improvement] Exit on OutOfMemoryError [#460][Improvement] Exit on OutOfMemoryError Dec 21, 2023
@zuston zuston changed the title [#460][Improvement] Exit on OutOfMemoryError [#460] improvement: Exit on OutOfMemoryError Dec 21, 2023
Comment thread bin/start-shuffle-server.sh Outdated
-XX:+PrintGCTimeStamps \
-XX:+CrashOnOutOfMemoryError \
-XX:+ExitOnOutOfMemoryError \
${OOM_DUMP_OPTS} \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can just use JAVA_OPTS, because users may need add more options.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It all ok, but I think there is "MAX_DIRECT_MEMORY_OPTS", so I added OOM_DUMP_OPTS.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I mean we can add a env variable like spark.driver.extraJavaOptions which in spark. So that users can add any arguments to JVM_ARGS by just add arguments to JAVA_OPTS .

@codecov-commenter

codecov-commenter commented Dec 21, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ecbf2e7) 53.22% compared to head (fd49f0b) 54.11%.
Report is 11 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1390      +/-   ##
============================================
+ Coverage     53.22%   54.11%   +0.89%     
- Complexity     2715     2720       +5     
============================================
  Files           418      399      -19     
  Lines         23932    21628    -2304     
  Branches       2043     2050       +7     
============================================
- Hits          12738    11705    -1033     
+ Misses        10408     9205    -1203     
+ Partials        786      718      -68     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

-XX:+PrintGCTimeStamps \
-XX:+CrashOnOutOfMemoryError \
-XX:+ExitOnOutOfMemoryError \
-XX:+PrintTenuringDistribution \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure if these parameters will affect performance. PTAL @zuston @jerqi

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We use this args in PRD, so PTAL.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm ok.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

roryqi
roryqi previously approved these changes Dec 21, 2023

@roryqi roryqi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

zuston
zuston previously approved these changes Dec 21, 2023

@zuston zuston left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@lifeSo
lifeSo dismissed stale reviews from zuston and roryqi via fd49f0b December 21, 2023 05:43
@zuston
zuston merged commit 386f795 into apache:master Dec 21, 2023
@lifeSo
lifeSo deleted the feature_oom_460 branch December 21, 2023 12:29
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.

[Improvement] Exit on OutOfMemoryError

5 participants