You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "scwhittle (via GitHub)" <gi...@apache.org> on 2024/10/28 12:47:10 UTC
[PR] Modify JVM options if enableHeapDumps is specified to dump heap in directory that MemoryMonitor will look in. [beam]
scwhittle opened a new pull request, #32953:
URL: https://github.com/apache/beam/pull/32953
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Modify JVM options if enableHeapDumps is specified to dump heap in directory that MemoryMonitor will look in. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32953:
URL: https://github.com/apache/beam/pull/32953#issuecomment-2564316572
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Java Harness] Improve heap dump support by configuring JVM to dump heap on OOM exceptions [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32953:
URL: https://github.com/apache/beam/pull/32953#issuecomment-2970214092
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment `assign set of reviewers`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Modify JVM options if enableHeapDumps is specified to dump heap in directory that MemoryMonitor will look in. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #32953: Modify JVM options if enableHeapDumps is specified to dump heap in directory that MemoryMonitor will look in.
URL: https://github.com/apache/beam/pull/32953
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Java Harness] Improve heap dump support by configuring JVM to dump heap on OOM exceptions [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on PR #32953:
URL: https://github.com/apache/beam/pull/32953#issuecomment-2980032361
R: @kennknowles
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Java Harness] Improve heap dump support by configuring JVM to dump heap on OOM exceptions [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on code in PR #32953:
URL: https://github.com/apache/beam/pull/32953#discussion_r2156856551
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/status/MemoryMonitor.java:
##########
@@ -237,14 +255,26 @@ private MemoryMonitor(
boolean canDumpHeap,
double gcThrashingPercentagePerPeriod,
@Nullable String uploadFilePath,
- File localDumpFolder) {
+ File localDumpFolder,
+ boolean gzipCompress) {
this.gcStatsProvider = gcStatsProvider;
this.sleepTimeMillis = sleepTimeMillis;
this.shutDownAfterNumGCThrashing = shutDownAfterNumGCThrashing;
- this.canDumpHeap = canDumpHeap;
this.gcThrashingPercentagePerPeriod = gcThrashingPercentagePerPeriod;
this.uploadFilePath = uploadFilePath;
this.localDumpFolder = localDumpFolder;
+ this.gzipCompress = gzipCompress;
+
+ if (canDumpHeap) {
+ try {
+ Files.createDirectories(localDumpFolder.toPath());
Review Comment:
moved to the fromOptions. More eagerly seems nice since we can log that it won't work which gives user more of a chance to fix things before they want to investigate ooms.
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/status/MemoryMonitor.java:
##########
@@ -573,6 +621,7 @@ private static File getLoggingDir() {
public File dumpHeap()
throws MalformedObjectNameException, InstanceNotFoundException, ReflectionException,
MBeanException, IOException {
+ Preconditions.checkState(canDumpHeap);
Review Comment:
done
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/status/MemoryMonitor.java:
##########
@@ -194,21 +196,35 @@ public long totalGCTimeMilliseconds() {
private final File localDumpFolder;
+ private final boolean gzipCompress;
+
public static MemoryMonitor fromOptions(PipelineOptions options) {
SdkHarnessOptions sdkHarnessOptions = options.as(SdkHarnessOptions.class);
- String uploadFilePath = options.getTempLocation();
+ @Nullable String uploadFilePath = sdkHarnessOptions.getRemoteHeapDumpLocation();
+ if (uploadFilePath == null) {
+ uploadFilePath = options.getTempLocation();
Review Comment:
done, added more tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Java Harness] Improve heap dump support by configuring JVM to dump heap on OOM exceptions [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #32953:
URL: https://github.com/apache/beam/pull/32953#discussion_r2153095892
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/status/MemoryMonitor.java:
##########
@@ -573,6 +621,7 @@ private static File getLoggingDir() {
public File dumpHeap()
throws MalformedObjectNameException, InstanceNotFoundException, ReflectionException,
MBeanException, IOException {
+ Preconditions.checkState(canDumpHeap);
Review Comment:
An error message here would be good, so if it ever happens the user will report it. Maybe something like "Bug! Attempt to dump heap even though it should be disabled."
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/status/MemoryMonitor.java:
##########
@@ -194,21 +196,35 @@ public long totalGCTimeMilliseconds() {
private final File localDumpFolder;
+ private final boolean gzipCompress;
+
public static MemoryMonitor fromOptions(PipelineOptions options) {
SdkHarnessOptions sdkHarnessOptions = options.as(SdkHarnessOptions.class);
- String uploadFilePath = options.getTempLocation();
+ @Nullable String uploadFilePath = sdkHarnessOptions.getRemoteHeapDumpLocation();
+ if (uploadFilePath == null) {
+ uploadFilePath = options.getTempLocation();
Review Comment:
Would be useful to have defensive tests of this code path. (aka a couple tests of `fromOptions` with different options set)
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/status/MemoryMonitor.java:
##########
@@ -237,14 +255,26 @@ private MemoryMonitor(
boolean canDumpHeap,
double gcThrashingPercentagePerPeriod,
@Nullable String uploadFilePath,
- File localDumpFolder) {
+ File localDumpFolder,
+ boolean gzipCompress) {
this.gcStatsProvider = gcStatsProvider;
this.sleepTimeMillis = sleepTimeMillis;
this.shutDownAfterNumGCThrashing = shutDownAfterNumGCThrashing;
- this.canDumpHeap = canDumpHeap;
this.gcThrashingPercentagePerPeriod = gcThrashingPercentagePerPeriod;
this.uploadFilePath = uploadFilePath;
this.localDumpFolder = localDumpFolder;
+ this.gzipCompress = gzipCompress;
+
+ if (canDumpHeap) {
+ try {
+ Files.createDirectories(localDumpFolder.toPath());
Review Comment:
I don't think we should be doing this kind of logic in the constructor. Perhaps in the static factory method, or lazily at heap dump time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Modify JVM options if enableHeapDumps is specified to dump heap in directory that MemoryMonitor will look in. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32953:
URL: https://github.com/apache/beam/pull/32953#issuecomment-2571278073
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Java Harness] Improve heap dump support by configuring JVM to dump heap on OOM exceptions [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on PR #32953:
URL: https://github.com/apache/beam/pull/32953#issuecomment-2970211588
R: @Abacn
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Java Harness] Improve heap dump support by configuring JVM to dump heap on OOM exceptions [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on PR #32953:
URL: https://github.com/apache/beam/pull/32953#issuecomment-2987863043
@kennknowles back to you . Sorry I rebased on accident and broke the diff view.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Java Harness] Improve heap dump support by configuring JVM to dump heap on OOM exceptions [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32953:
URL: https://github.com/apache/beam/pull/32953#issuecomment-2966785716
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Java Harness] Improve heap dump support by configuring JVM to dump heap on OOM exceptions [beam]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #32953:
URL: https://github.com/apache/beam/pull/32953#issuecomment-2969553011
## [Codecov](https://app.codecov.io/gh/apache/beam/pull/32953?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `7 lines` in your changes missing coverage. Please review.
> Project coverage is 54.49%. Comparing base [(`de76cfa`)](https://app.codecov.io/gh/apache/beam/commit/de76cfa16d36e2cb8dd1a1092710f0cd477e0f8d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`5d3b011`)](https://app.codecov.io/gh/apache/beam/commit/5d3b011d110416d75415073c8591e5203cfcba95?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 26 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/beam/pull/32953?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [sdks/java/container/boot.go](https://app.codecov.io/gh/apache/beam/pull/32953?src=pr&el=tree&filepath=sdks%2Fjava%2Fcontainer%2Fboot.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9qYXZhL2NvbnRhaW5lci9ib290Lmdv) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/beam/pull/32953?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #32953 +/- ##
============================================
- Coverage 54.50% 54.49% -0.01%
+ Complexity 1479 1470 -9
============================================
Files 1017 1017
Lines 160983 160780 -203
Branches 1079 1078 -1
============================================
- Hits 87737 87614 -123
+ Misses 71140 71056 -84
- Partials 2106 2110 +4
```
| [Flag](https://app.codecov.io/gh/apache/beam/pull/32953/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [go](https://app.codecov.io/gh/apache/beam/pull/32953/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `28.20% <0.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/beam/pull/32953?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org