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.

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

removing the explicit System.exit(0) from Main #3400

Merged
merged 2 commits into from
Aug 8, 2017

Conversation

lbergelson
Copy link
Member

this fixes the yarn early termination bug reported in #2666 and #3166

this fixes the yarn early termination bug reported in #2666 and #3166
@lbergelson
Copy link
Member Author

@droazen @magicDGS Can any of you think of a reason this would cause problems? It seems like it should just continue and exit with 0 unless there's a non-daemon thread holding it open which is either something deliberate that should stay open until it's done or a bug.

@codecov-io
Copy link

codecov-io commented Aug 1, 2017

Codecov Report

Merging #3400 into master will increase coverage by 0.008%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##              master     #3400       +/-   ##
===============================================
+ Coverage     80.469%   80.477%   +0.008%     
- Complexity     17522     17533       +11     
===============================================
  Files           1173      1173               
  Lines          63402     63459       +57     
  Branches        9879      9894       +15     
===============================================
+ Hits           51019     51070       +51     
- Misses          8434      8441        +7     
+ Partials        3949      3948        -1
Impacted Files Coverage Δ Complexity Δ
.../main/java/org/broadinstitute/hellbender/Main.java 52.571% <ø> (+0.299%) 25 <0> (ø) ⬇️
...stitute/hellbender/tools/walkers/vqsr/Tranche.java 62.921% <0%> (-7.349%) 18% <0%> (ø)
.../hellbender/tools/walkers/vqsr/TrancheManager.java 67.347% <0%> (-2.983%) 18% <0%> (+3%)
...oadinstitute/hellbender/utils/gcs/BucketUtils.java 74.342% <0%> (+1.974%) 36% <0%> (ø) ⬇️
...e/hellbender/engine/spark/SparkContextFactory.java 70% <0%> (+3.333%) 10% <0%> (ø) ⬇️
...e/hellbender/tools/walkers/vqsr/VQSLODTranche.java 91.111% <0%> (+7.273%) 25% <0%> (+8%) ⬆️

@magicDGS
Copy link
Contributor

magicDGS commented Aug 2, 2017

For me this is a 👍 (I will complain as always if it does not work for me, no worries), because I think that this should work.

Just in case, it will be useful to add a documentation note in handleResult() suggesting that if it needs to be explicitly call it should be added there.

@droazen
Copy link
Contributor

droazen commented Aug 2, 2017

@lbergelson Is there a way we can test this fix?

Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

👍 one comment, then merge when ready

@droazen droazen assigned lbergelson and unassigned droazen Aug 2, 2017
@lbergelson
Copy link
Member Author

@david-wb @popboy126 I'm having trouble reproducing the shutdown issue on our own cluster, I sometimes get the message Shutdown hook called before final status was reported. but the job status is SUCCEEDED. This happens if I run with the System.exit(0) or not. Could one of you test with this branch and let me know if it solves your issue? I think my cluster configuration must be different then yours in some way.

@lbergelson
Copy link
Member Author

I haven't been able to reproduce the issue myself, and I haven't heard back from either of the people who reported this issue, but it seems likely that this will solve it so I'm going to merge and they can reopen if they see it again.

@lbergelson lbergelson merged commit 0565ee6 into master Aug 8, 2017
@lbergelson lbergelson deleted the lb_fix_yarn_container_early_exit branch August 8, 2017 20:19
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