Shading SLF4J prevents SLF4J locating org.slf4j.impl.StaticLoggerBinder

[asfimport]

Parquet-format shades SLF4J to parquet.org.slf4j (see here). This also accidentally shades this line

private static String STATIC_LOGGER_BINDER_PATH = "org/slf4j/impl/StaticLoggerBinder.class";

to

private static String STATIC_LOGGER_BINDER_PATH = "parquet/org/slf4j/impl/StaticLoggerBinder.class";

and thus LoggerFactory can never find the correct StaticLoggerBinder implementation even if we provide dependencies like slf4j-log4j12 on the classpath.

This happens in Spark. Whenever we write a Parquet file, we see the following famous message and can never get rid of it:

SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.

Reporter: Cheng Lian / @liancheng
Assignee: Ryan Blue / @rdblue

Related issues:

• Deprecate Log and move to SLF4J Logger (relates to)
• Shutdown hook in parquet-avro library corrupts data and disables logging (relates to)
• Format: Do not shade slf4j-api (relates to)

PRs and other links:

• Format PR #32

_{Note: This issue was originally created as PARQUET-369. Please see the migration documentation for further details.}

[asfimport]

Jonathan Coveney / @jcoveney:
Nice! I had been wondering what was causing that message in spark.

[asfimport]

Cheng Lian / @liancheng:
Found this old Google groups post by @julienledem. I think it should be OK to just stop shading SLF4J in parquet-format.

[asfimport]

Cheng Lian / @liancheng:
Here is a more concrete version in another thread https://groups.google.com/d/msg/parquet-dev/UjpbHbzoQj0/R6LG2gECQuIJ

@julienledem According to the link above, is testing the only reason why we have the static JUL initialization block in parquet-mr? If that is true, I'm happy to have a try to remove it. We've been using pretty hacky way in Spark to redirect Parquet JUL logs.

[asfimport]

Ryan Blue / @rdblue:
I've fixed this by using an exclusion to not shade the org.slf4j.impl.* classes. There aren't any impl classes in the jar so there are no leaked org.slf4j classes, but this prevents references to log bindings from being modified.

Not shading slf4j would be a problem because parquet-format would have a dependency on it. I also tried minimizing the jar to see if it went away entirely, but slf4j was still required because classes in thrift still reference it. I think this is a good fix.

[asfimport]

Ryan Blue / @rdblue:
I should also note: I've verified that there are no org.slf4j.* classes in the shaded parquet-format jar (they are now shaded.parquet.org.slf4j) and I decompiled LoggerFactory and verified that the reference to StaticLoggerBinder.class is unmodified.

[asfimport]

Ryan Blue / @rdblue:
Ignore my comment above, I just tested out the partial relocation and it doesn't work because of references back to some of the moved classes. Looks like we can either ship parquet-format with a slf4j-api dependency or bundle it with a logger implementation, like slf4j-nop.

I don't think there is much interesting information being logged by thrift, plus we haven't been able to get those messages for at least the last release without complaints. I suggest we add slf4j-nop and shade that to avoid the warning.

[asfimport]

Ryan Blue / @rdblue:
I've updated the PR to shade slf4j-nop and confirmed that everything still works, but the warning is gone.

[asfimport]

Konstantin Shaposhnikov / @kostya-sh:
What is the reason to include shaded slf4j-api and slf4j-nop into parquet-format jar?

slf4j is not used by parquet-format (but it looks like it is used by libtrift)

I think it would be much better to not shade these jars and let the application decide which logging framework to use. Including slf4j-nop effectively silence all logging from libthrift.

[asfimport]

Ryan Blue / @rdblue:
[~k.shaposhnikov@gmail.com], we could either shade and silence logging from libthrift or we could pass on the dependency as you suggest. In my opinion, silencing is the better option because needing access to the logging done by libthrift is rare for Parquet. I would much rather not pollute the dependency graph, especially when we are using such an old version of thrift and whatever slf4j version was out at that time.

[asfimport]

Michal Turek / @mixalturek:
Please see comment https://issues.apache.org/jira/browse/PARQUET-401?focusedCommentId=15057800&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15057800 and optionally consider to reopen this issue.

[asfimport]

Ryan Blue / @rdblue:
The issue [~turek@avast.com] raises is basically that shading both slf4j-api and slf4j-nop disables logging from thrift entirely. (Only thrift is affected because the Parquet code doesn't contain any logging.) Specifically, that will suppress warnings for IOExceptions caught in TIOStreamTransport#close. That's the same problem that [~k.shaposhnikov@gmail.com] brought up.

I'm still okay with this change. I think that it is rare for Thrift to suppress exceptions and log them and the right solution is to allow exceptions to propagate. That behavior isn't going to change, but I still don't think exceptions that happen on close are worth adding a dependency on SLF4J.

But... this has been flagged twice. In addition, Parquet MR now depends on SLF4J for its logging (PARQUET-305), so it may not make as much sense to avoid the dependency from parquet-format any more. As long as any user of Parquet MR has a dependency on SLF4J, why not add it here also? The version is old but we could add a direct dependency on a newer SLF4J to override the transitive dependency.

@julienledem, @danielcweeks, @isnotinvain, [~lian cheng], what do you guys think?

[asfimport]

Alex Levenson / @isnotinvain:
Can we just replace all logging w/ java.util.logging? Does that simplify or no?

I find that working with slf4j, log4j, and the 12 other jvm logging libraries + bridges is a lot of pain to just wind up with everything going to stderr anyway because you can never get the 12 different config files just right or on the classpath correctly or whatever it is that makes them work. I'm not that familiar with actually getting these things to work.

[asfimport]

Ryan Blue / @rdblue:
Using java.util.logging doesn't simplify it. I think that the right way to simplify things is to use SLF4J because its purpose is to provide a good/standard API and also allow you to swap out the actual logging framework for whatever you want. It's basically what we were trying to do in the Log class but done for us. SLF4J also has a benefit of being pretty much standard for Java these days, which is why our dependencies use it.

The solution we've committed for Parquet MR is to use the SLF4J API and allow downstream consumers to choose the right underlying framework for them by depending on slf4j-log4j or something similar. All that is being proposed here is to expose the dependency from parquet-format so that those logs aren't lost. The trade-off is exposing the dependency vs. losing what is logged by thrift for the format classes. Before, we opted to remove the dependency but now that all of Parquet MR uses SLF4J, I'm coming around to the idea that we might want to expose it from parquet-format.

[asfimport]

Jacques Nadeau / @jacques-n:
+1 for Ryan's suggestion. Not sure how many Java users exist that depend on the format but not MR (are there any?). Might as well use slf4j for both.