Logging to be done by ADAM utils code rather than Spark #1028

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@jpdna
Member

jpdna commented May 3, 2016

This PR has a dependency on bdg-utils PR at:
bigdatagenomics/utils#72
and will fail tests without it.

Note: also bumps Spark version 1.6.1

This PR solves the version compatibility logging problems caused by recent versions of Spark making org.apache.spark.Logging private to Spark

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins May 3, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1225/

Build result: FAILURE

GitHub pull request #1028 of commit 78df340 automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1028/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains d24066a # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1028/merge^{commit} # timeout=10Checking out Revision d24066a (origin/pr/1028/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f d24066a689ff8492b3fd2a73af18b22e733d134aFirst time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1225/

Build result: FAILURE

GitHub pull request #1028 of commit 78df340 automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1028/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains d24066a # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1028/merge^{commit} # timeout=10Checking out Revision d24066a (origin/pr/1028/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f d24066a689ff8492b3fd2a73af18b22e733d134aFirst time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins May 3, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1226/

Build result: FAILURE

GitHub pull request #1028 of commit 47ed598 automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1028/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains 02382165b57f87412b4bee672838726a3ce6a815 # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1028/merge^{commit} # timeout=10Checking out Revision 02382165b57f87412b4bee672838726a3ce6a815 (origin/pr/1028/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 02382165b57f87412b4bee672838726a3ce6a815First time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1226/

Build result: FAILURE

GitHub pull request #1028 of commit 47ed598 automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1028/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains 02382165b57f87412b4bee672838726a3ce6a815 # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1028/merge^{commit} # timeout=10Checking out Revision 02382165b57f87412b4bee672838726a3ce6a815 (origin/pr/1028/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 02382165b57f87412b4bee672838726a3ce6a815First time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@@ -21,12 +21,12 @@
<avro.version>1.7.7</avro.version>
<!-- informative only, used in about template substitution -->
<scala.version>2.10</scala.version>
- <spark.version>1.5.2</spark.version>
+ <spark.version>1.6.1</spark.version>

This comment has been minimized.

@heuermh

heuermh May 3, 2016

Member

Might want to bump this in a separate pull request to test separately.

@heuermh

heuermh May 3, 2016

Member

Might want to bump this in a separate pull request to test separately.

This comment has been minimized.

@fnothaft

fnothaft May 3, 2016

Member

Similar to my comment upstream, I would just split this out into a separate commit.

@fnothaft

fnothaft May 3, 2016

Member

Similar to my comment upstream, I would just split this out into a separate commit.

pom.xml
@@ -370,6 +370,19 @@
</exclusion>
</exclusions>
</dependency>
+

This comment has been minimized.

@fnothaft

fnothaft May 3, 2016

Member

Nit: extra spacing around this addition.

@fnothaft

fnothaft May 3, 2016

Member

Nit: extra spacing around this addition.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins May 4, 2016

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1227/

Build result: FAILURE

GitHub pull request #1028 of commit 33f4756 automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1028/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains 9f663a6 # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1028/merge^{commit} # timeout=10Checking out Revision 9f663a6 (origin/pr/1028/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 9f663a6d1bbbdd982250e259988648e57e367079First time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1227/

Build result: FAILURE

GitHub pull request #1028 of commit 33f4756 automatically merged.Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'[EnvInject] - Loading node environment variables.Building remotely on amp-jenkins-worker-05 (centos spark-test) in workspace /home/jenkins/workspace/ADAM-prb > /home/jenkins/git2/bin/git rev-parse --is-inside-work-tree # timeout=10Fetching changes from the remote Git repository > /home/jenkins/git2/bin/git config remote.origin.url https://github.com/bigdatagenomics/adam.git # timeout=10Fetching upstream changes from https://github.com/bigdatagenomics/adam.git > /home/jenkins/git2/bin/git --version # timeout=10 > /home/jenkins/git2/bin/git -c core.askpass=true fetch --tags --progress https://github.com/bigdatagenomics/adam.git +refs/pull/:refs/remotes/origin/pr/ # timeout=15 > /home/jenkins/git2/bin/git rev-parse origin/pr/1028/merge^{commit} # timeout=10 > /home/jenkins/git2/bin/git branch -a --contains 9f663a6 # timeout=10 > /home/jenkins/git2/bin/git rev-parse remotes/origin/pr/1028/merge^{commit} # timeout=10Checking out Revision 9f663a6 (origin/pr/1028/merge) > /home/jenkins/git2/bin/git config core.sparsecheckout # timeout=10 > /home/jenkins/git2/bin/git checkout -f 9f663a6d1bbbdd982250e259988648e57e367079First time build. Skipping changelog.Triggering ADAM-prb ? 2.6.0,2.10,1.5.2,centosTriggering ADAM-prb ? 2.6.0,2.11,1.5.2,centosTouchstone configurations resulted in FAILURE, so aborting...Notifying endpoint 'HTTP:https://webhooks.gitter.im/e/ac8bb6e9f53357bc8aa8'
Test FAILed.

@jpdna

This comment has been minimized.

Show comment
Hide comment
@jpdna

jpdna May 4, 2016

Member

Should be ready to go now - along with its utils dependency bigdatagenomics/utils#72

Member

jpdna commented May 4, 2016

Should be ready to go now - along with its utils dependency bigdatagenomics/utils#72

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft May 5, 2016

Member

@jpdna bdg-utils 0.2.5 is released and available through Maven Central now: http://search.maven.org/#artifactdetails%7Corg.bdgenomics.utils%7Cutils-parent_2.10%7C0.2.5%7Cpom

Can you modify the POMs to point at this? Once you're done and tests pass, I can merge.

Member

fnothaft commented May 5, 2016

@jpdna bdg-utils 0.2.5 is released and available through Maven Central now: http://search.maven.org/#artifactdetails%7Corg.bdgenomics.utils%7Cutils-parent_2.10%7C0.2.5%7Cpom

Can you modify the POMs to point at this? Once you're done and tests pass, I can merge.

@jpdna

This comment has been minimized.

Show comment
Hide comment
@jpdna

jpdna May 5, 2016

Member

I don't think the 0.2.5 utils release pushed to Maven Central contained my Logging PR as intended - and doesn't match https://github.com/bigdatagenomics/utils

I cleared Utils out of my local mvn repo to be sure that I pulled down the public one, but find that there is no Logging class in utils-misc_2.10-0.2.5.jar

Member

jpdna commented May 5, 2016

I don't think the 0.2.5 utils release pushed to Maven Central contained my Logging PR as intended - and doesn't match https://github.com/bigdatagenomics/utils

I cleared Utils out of my local mvn repo to be sure that I pulled down the public one, but find that there is no Logging class in utils-misc_2.10-0.2.5.jar

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft May 6, 2016

Member

@jpdna just pushed a fixed bdg-utils release. It should propagate to Maven Central in the next few hours.

Member

fnothaft commented May 6, 2016

@jpdna just pushed a fixed bdg-utils release. It should propagate to Maven Central in the next few hours.

@jpdna

This comment has been minimized.

Show comment
Hide comment
@jpdna

jpdna May 16, 2016

Member

@fnothaft - it looks to me like the utils-misc_2.10-0.2.5 that we get from Maven Central still does not contain the Logging class as intended - so still is the old version which didn't include that commit which added Logging.
I've double checkout downloading directly from maven repo, and it doesn't seem to be there.
Any steps I should take to resolve this?

Member

jpdna commented May 16, 2016

@fnothaft - it looks to me like the utils-misc_2.10-0.2.5 that we get from Maven Central still does not contain the Logging class as intended - so still is the old version which didn't include that commit which added Logging.
I've double checkout downloading directly from maven repo, and it doesn't seem to be there.
Any steps I should take to resolve this?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft May 16, 2016

Member

@jpdna You'll want to look at 0.2.6. Once a release is pushed to Maven Central, I can't overwrite it, so I had to cut a new point release.

Member

fnothaft commented May 16, 2016

@jpdna You'll want to look at 0.2.6. Once a release is pushed to Maven Central, I can't overwrite it, so I had to cut a new point release.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins May 16, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1231/
Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1231/
Test PASSed.

@jpdna

This comment has been minimized.

Show comment
Hide comment
@jpdna

jpdna May 16, 2016

Member

This should be ready to merge now if someone can take a look. Thanks!

Member

jpdna commented May 16, 2016

This should be ready to merge now if someone can take a look. Thanks!

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh May 16, 2016

Member

The diff looks reasonable to me. Should any of the commits be merged?

Since we've left out the default Spark logging configuration, I wonder how different the logging experience will be for our users. UCSD had some complaints in this regard. I spent a little time looking into it before, and came away without a solution or even a good understanding what was going on. Would we want to look into that as part of this pull request or a follow on?

Member

heuermh commented May 16, 2016

The diff looks reasonable to me. Should any of the commits be merged?

Since we've left out the default Spark logging configuration, I wonder how different the logging experience will be for our users. UCSD had some complaints in this regard. I spent a little time looking into it before, and came away without a solution or even a good understanding what was going on. Would we want to look into that as part of this pull request or a follow on?

@jpdna

This comment has been minimized.

Show comment
Hide comment
@jpdna

jpdna May 16, 2016

Member

I have need for this PR in relation to the other Spark 2.0 work, so if the current behavior still seems at least as good as status quo, which it does to me, then I'd vote for getting this PR in sooner and revisiting the issue of further improving login later.

Wrt to merging the commits, I was splitting out the version bumps as previously requested - so suggest to keep these 3 individual commits seperate as they are now.

Member

jpdna commented May 16, 2016

I have need for this PR in relation to the other Spark 2.0 work, so if the current behavior still seems at least as good as status quo, which it does to me, then I'd vote for getting this PR in sooner and revisiting the issue of further improving login later.

Wrt to merging the commits, I was splitting out the version bumps as previously requested - so suggest to keep these 3 individual commits seperate as they are now.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft May 16, 2016

Member

LGTM. I think 062caaa and 33f4756 should be squashed, since 33f4756 can't be compiled standalone.

Member

fnothaft commented May 16, 2016

LGTM. I think 062caaa and 33f4756 should be squashed, since 33f4756 can't be compiled standalone.

@AmplabJenkins

This comment has been minimized.

Show comment
Hide comment
@AmplabJenkins

AmplabJenkins May 16, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1232/
Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1232/
Test PASSed.

@jpdna

This comment has been minimized.

Show comment
Hide comment
@jpdna

jpdna May 16, 2016

Member

think 062caaa and 33f4756 should be squashed, since 33f4756 can't be compiled standalone.

Done

Member

jpdna commented May 16, 2016

think 062caaa and 33f4756 should be squashed, since 33f4756 can't be compiled standalone.

Done

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh May 16, 2016

Member

+1

Member

heuermh commented May 16, 2016

+1

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh May 17, 2016

Member

@fnothaft could you merge this? the commit script expects only 1 commit

Member

heuermh commented May 17, 2016

@fnothaft could you merge this? the commit script expects only 1 commit

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft May 18, 2016

Member

Working on the merge now.

Member

fnothaft commented May 18, 2016

Working on the merge now.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft May 18, 2016

Member

Thanks @jpdna! Merged manually as 728459c and 33841c0.

Member

fnothaft commented May 18, 2016

Thanks @jpdna! Merged manually as 728459c and 33841c0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment