pkumar-singh commented on a change in pull request #2816:
URL: https://github.com/apache/bookkeeper/pull/2816#discussion_r729987757
##########
File path: bookkeeper-dist/server/pom.xml
##########
@@ -85,8 +85,16 @@
<!-- slf4j binding -->
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
+ <groupId>org.apache.logging.log4j</groupId>
Review comment:
I know I am late at the PR, apologies. But I am not sure this is the
right thing to do. Bookkeeper should work with SLF4J instead of log4j. Allow me
to explain. There are three elements to it.
1. SLF4J this is the facade for logging framework. It does not provide
logging implementation. But just facade API. Bookkeeper should only be using
that, test code could be a possible exception. That way SLF4j can work with
any type of logging infra, log4j2 or log4j1 being just few options out of many.
2. SLF4j-LOG4J12: This is Binding for SLF4 facade with LOG4J1, "ideally"
bookkeeper should not have any dependency on this. This should again be
provided by application at runtime. Role of SLF4j-LOG4J12 is that If SLF4J sees
SLF4j-LOG4J12 in the Runtime classpath, It will go on lookout for log4j12. And
that will ensure that for actual logging log4j2 or log4j12 is used. In other
words SLF4j-LOG4J12 works as bridge between facade and implementation.
3. log4j12 or log4j2 or simplelog4j or foolog4j: This is actual logging
library. Which does the actual logging. And certainly should be provided at
runtime by application. In ideal case Bookkeeper should not provide this as
dependency.
To me looks like we have removed slf4j and started using log4j directly.
Which means we have created a hard binding with log4j, which I believe is not
desirable.
@eolivelli @RaulGracia @merlimat @ivankelly @hsaputra
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
