-
Notifications
You must be signed in to change notification settings - Fork 594
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
Spark script improvements #5815
Conversation
- Increase master disk size - Use Dataproc 1.3 - Use gz for known sites - Script for running genome dataset on gcs
Codecov Report
@@ Coverage Diff @@
## master #5815 +/- ##
===============================================
+ Coverage 87.029% 87.033% +0.003%
- Complexity 32104 32109 +5
===============================================
Files 1972 1972
Lines 147187 147194 +7
Branches 16201 16201
===============================================
+ Hits 128096 128107 +11
+ Misses 13184 13181 -3
+ Partials 5907 5906 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are mostly just a better accounting of what you need to run your own spark tests, and consequently I haven't much to say. I am a little concerned that changing the scripts to leave a cluster alive is a little dangerous for people who don't know what they are doing. Something should probably be done to soften that blow.
--max-age 3h \ | ||
--project broad-gatk-collab | ||
|
||
# Run scripts | ||
for script in "$@" | ||
do | ||
SCRIPT_NAME="$script" | ||
source "$script" | ||
eval "$script" || exit $? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should loudly inform the user that it has elected to keep their cluster alive. This seems like a dangerous change to make to the script for some developers compute costs. Could you start the cluster with a time to live perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jamesemery. The cluster already has a max age of 3 hours. I've added a message to say the cluster won't be deleted immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Fixes #5152