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

ADAM specific logging #1024

Closed
jpdna opened this Issue Apr 28, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@jpdna
Member

jpdna commented Apr 28, 2016

Due to Spark 2.0 moving to org.apache.spark.internal.Logging which is private to Spark, we need to have our own Logging trait specific to ADAM.

I'd propose to do this by creating an
org.bdgenomics.adam.Logging trait and companion object modeled after:
https://github.com/apache/spark/blob/branch-1.6/core/src/main/scala/org/apache/spark/Logging.scala

If anyone has an alternative suggestion, please let me know.

@jpdna jpdna self-assigned this Apr 28, 2016

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Apr 28, 2016

Member

I would rather open this upstream in org.bdgenomics.utils.misc.

Member

fnothaft commented Apr 28, 2016

I would rather open this upstream in org.bdgenomics.utils.misc.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Apr 28, 2016

Member

Otherwise, I am +1.

Member

fnothaft commented Apr 28, 2016

Otherwise, I am +1.

@jpdna

This comment has been minimized.

Show comment
Hide comment
@jpdna

jpdna Apr 28, 2016

Member

I would rather open this upstream in org.bdgenomics.utils.misc.

makes sense, will do

Member

jpdna commented Apr 28, 2016

I would rather open this upstream in org.bdgenomics.utils.misc.

makes sense, will do

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Apr 28, 2016

Member

+1 to bgd-utils

Member

heuermh commented Apr 28, 2016

+1 to bgd-utils

@jpdna

This comment has been minimized.

Show comment
Hide comment
@jpdna

jpdna Apr 28, 2016

Member

This logging stuff is gonna be more of a pain then I thought....
I guess we now pay the price for not heeding the warning:
https://github.com/apache/spark/blob/branch-1.6/core/src/main/scala/org/apache/spark/Logging.scala#L32

I had initially planned to make our ADAM version of logging by basically copy and paste code from the old externally accessible version of spark Logger at
https://github.com/apache/spark/blob/branch-1.6/core/src/main/scala/org/apache/spark/Logging.scala
into package org.bdgenomics.utils.misc

That almost works, but to make logging work with REPL Logger contains:
https://github.com/apache/spark/blob/branch-1.6/core/src/main/scala/org/apache/spark/Logging.scala#L124
which uses org.apache.spark.utils.Utils which itself was already private to Spark in 1.6

So - to continue on this path we will also need to create an ADAM copy of all applicable parts of org.apache.spark.util
if we are going to make ADAM logging continue to work in the same way.

I think this will be possible, but copying and pasting lots of code here smells bad.
Let me know if you see a better way - otherwise I will push on with copying logging related code from Spark to ADAM.

Member

jpdna commented Apr 28, 2016

This logging stuff is gonna be more of a pain then I thought....
I guess we now pay the price for not heeding the warning:
https://github.com/apache/spark/blob/branch-1.6/core/src/main/scala/org/apache/spark/Logging.scala#L32

I had initially planned to make our ADAM version of logging by basically copy and paste code from the old externally accessible version of spark Logger at
https://github.com/apache/spark/blob/branch-1.6/core/src/main/scala/org/apache/spark/Logging.scala
into package org.bdgenomics.utils.misc

That almost works, but to make logging work with REPL Logger contains:
https://github.com/apache/spark/blob/branch-1.6/core/src/main/scala/org/apache/spark/Logging.scala#L124
which uses org.apache.spark.utils.Utils which itself was already private to Spark in 1.6

So - to continue on this path we will also need to create an ADAM copy of all applicable parts of org.apache.spark.util
if we are going to make ADAM logging continue to work in the same way.

I think this will be possible, but copying and pasting lots of code here smells bad.
Let me know if you see a better way - otherwise I will push on with copying logging related code from Spark to ADAM.

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh Apr 28, 2016

Member

What does this façade provide beyond that already provided by slf4j Logger? As long as ours are consistently marked @transient I don't see much there.

Member

heuermh commented Apr 28, 2016

What does this façade provide beyond that already provided by slf4j Logger? As long as ours are consistently marked @transient I don't see much there.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Apr 28, 2016

Member

I am a-OK with dropping REPL logger optimization (what does that even mean? it looks like it just switches the logging config if you are in the REPL) if it simplifies the logger.

Member

fnothaft commented Apr 28, 2016

I am a-OK with dropping REPL logger optimization (what does that even mean? it looks like it just switches the logging config if you are in the REPL) if it simplifies the logger.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft May 18, 2016

Member

Closed by 33841c0.

Member

fnothaft commented May 18, 2016

Closed by 33841c0.

@fnothaft fnothaft closed this May 18, 2016

@heuermh heuermh referenced this issue Jun 7, 2016

Closed

Release ADAM version 0.20.0 #1048

47 of 61 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment