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

Remove adam-cli jar from classpath during adam-submit #749

Merged
merged 1 commit into from Aug 7, 2015

Conversation

Projects
None yet
5 participants
@fnothaft
Member

fnothaft commented Jul 31, 2015

The ./bin/compute-adam-jars.sh script does not remote the adam-cli JAR from the classpath. This exposes SPARK-1921 when running on YARN.

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Aug 3, 2015

Member

So this undoes the "fix" #669 of #663. Can you say more about how this exposes SPARK-1921 @fnothaft?

Member

ryan-williams commented Aug 3, 2015

So this undoes the "fix" #669 of #663. Can you say more about how this exposes SPARK-1921 @fnothaft?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 3, 2015

Member

Ah! Good catch @ryan-williams. You all are running Spark-on-YARN on a CDH5 setup at Mt. Sinai, right? Perhaps the behavior is different between our clusters (which wouldn't be surprising, I think the bug is caused by a race condition), but we are in the middle of bringing up a CDH5 cluster at Berkeley and saw the exact behavior described in SPARK-1921 (JAR timestamps differ). By removing the adam-cli jar from the JARS list when calling spark-submit, you don't copy the JAR twice, and thus don't have the timestamp race. It should be easy to make this work for both adam-submit and adam-shell; I'll fix this up.

Member

fnothaft commented Aug 3, 2015

Ah! Good catch @ryan-williams. You all are running Spark-on-YARN on a CDH5 setup at Mt. Sinai, right? Perhaps the behavior is different between our clusters (which wouldn't be surprising, I think the bug is caused by a race condition), but we are in the middle of bringing up a CDH5 cluster at Berkeley and saw the exact behavior described in SPARK-1921 (JAR timestamps differ). By removing the adam-cli jar from the JARS list when calling spark-submit, you don't copy the JAR twice, and thus don't have the timestamp race. It should be easy to make this work for both adam-submit and adam-shell; I'll fix this up.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 3, 2015

Member

@ryan-williams just pushed a new commit to address your comment. Doing things correctly actually makes the adam-submit code a bit cleaner, for certain definitions of clean (specifically, we get rid of the brittle way of determining the CLI jar). Let me know what you think, and I'll squash down and rebase.

Member

fnothaft commented Aug 3, 2015

@ryan-williams just pushed a new commit to address your comment. Doing things correctly actually makes the adam-submit code a bit cleaner, for certain definitions of clean (specifically, we get rid of the brittle way of determining the CLI jar). Let me know what you think, and I'll squash down and rebase.

Show outdated Hide outdated bin/adam-submit
@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Aug 3, 2015

Member

this lgtm btw, unless I've persuaded you here to push CLI-jar detection and removal into compute-adam-jars.sh

Member

ryan-williams commented Aug 3, 2015

this lgtm btw, unless I've persuaded you here to push CLI-jar detection and removal into compute-adam-jars.sh

@massie

This comment has been minimized.

Show comment
Hide comment
@massie

massie Aug 4, 2015

Member

@fnothaft Do you plan to move cli-jar detection tin compute-adam-jars.sh or is this ready to be merged?

@ryan-williams If this is ready to be merged, feel free to hit the button and merge it.

Member

massie commented Aug 4, 2015

@fnothaft Do you plan to move cli-jar detection tin compute-adam-jars.sh or is this ready to be merged?

@ryan-williams If this is ready to be merged, feel free to hit the button and merge it.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 4, 2015

Member

This isn't ready to merge yet; I still need to make fixes.

Member

fnothaft commented Aug 4, 2015

This isn't ready to merge yet; I still need to make fixes.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 5, 2015

Member

@ryan-williams just pushed a new round of fixes. Let me know what you think of these.

Member

fnothaft commented Aug 5, 2015

@ryan-williams just pushed a new round of fixes. Let me know what you think of these.

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Aug 5, 2015

Member

Builds failed with

hudson.plugins.git.GitException: Could not checkout null with start point 49a466a4d424b6acd1730c5daafe12f8e40d26b4

idk what that's about. Looking at the code now

Member

ryan-williams commented Aug 5, 2015

Builds failed with

hudson.plugins.git.GitException: Could not checkout null with start point 49a466a4d424b6acd1730c5daafe12f8e40d26b4

idk what that's about. Looking at the code now

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 5, 2015

Member

Jenkins, retest this please.

@ryan-williams there's a bug in the Jenkins Git plugin where fetching force-pushes fails occasionally.

Member

fnothaft commented Aug 5, 2015

Jenkins, retest this please.

@ryan-williams there's a bug in the Jenkins Git plugin where fetching force-pushes fails occasionally.

Show outdated Hide outdated bin/adam-submit
@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 7, 2015

Member

Just pushed a last commit with Michael's suggested change, and with all things squashed down and rebased on master. This should be good to merge.

Member

fnothaft commented Aug 7, 2015

Just pushed a last commit with Michael's suggested change, and with all things squashed down and rebased on master. This should be good to merge.

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams
Member

ryan-williams commented Aug 7, 2015

lgtm

@massie

This comment has been minimized.

Show comment
Hide comment
@massie

massie Aug 7, 2015

Member

+1

(Feel free to merge @ryan-williams)

Member

massie commented Aug 7, 2015

+1

(Feel free to merge @ryan-williams)

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Aug 7, 2015

Member

the latest diff looks like it adds back the $ADDL_JARS stuff -- was this intentional?
https://github.com/fnothaft/adam/blob/fix-compute-jars/bin/adam-submit#L54

Member

heuermh commented Aug 7, 2015

the latest diff looks like it adds back the $ADDL_JARS stuff -- was this intentional?
https://github.com/fnothaft/adam/blob/fix-compute-jars/bin/adam-submit#L54

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 7, 2015

Member

@heuermh thanks for the catch; not intentional, rather this comes from a rebase gone wrong... I will fix.

Member

fnothaft commented Aug 7, 2015

@heuermh thanks for the catch; not intentional, rather this comes from a rebase gone wrong... I will fix.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 7, 2015

Member

Fixed. Sorry; the rebase on #754 was kind of nasty; lots of messy conflicts.

Member

fnothaft commented Aug 7, 2015

Fixed. Sorry; the rebase on #754 was kind of nasty; lots of messy conflicts.

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Aug 7, 2015

Member

want to retest this @fnothaft ? looks like git force-push problem again

Member

ryan-williams commented Aug 7, 2015

want to retest this @fnothaft ? looks like git force-push problem again

@ryan-williams

This comment has been minimized.

Show comment
Hide comment
@ryan-williams

ryan-williams Aug 7, 2015

Member

good catch @heuermh btw! I looked at it and just assumed it was intentional, woops

Member

ryan-williams commented Aug 7, 2015

good catch @heuermh btw! I looked at it and just assumed it was intentional, woops

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 7, 2015

Member

Jenkins, test this please.

Member

fnothaft commented Aug 7, 2015

Jenkins, test this please.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Aug 7, 2015

Member

This is passing now; can I get a merge? I will rebase #757 once this is merged.

Member

fnothaft commented Aug 7, 2015

This is passing now; can I get a merge? I will rebase #757 once this is merged.

ryan-williams added a commit that referenced this pull request Aug 7, 2015

Merge pull request #749 from fnothaft/fix-compute-jars
Remove adam-cli jar from classpath during adam-submit

@ryan-williams ryan-williams merged commit 1cf8ebb into bigdatagenomics:master Aug 7, 2015

1 check passed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment