On 29.01.2010, at 04:43, Ralph Goers wrote:
On Jan 28, 2010, at 3:06 AM, Joern Huxhorn wrote:
Hi Ralph,
On 28.01.2010, at 08:42, Ralph Goers wrote:
In reading your response I'm not sure if you are just describing
how you originally thought this should be done or critiquing what
I've done. In your analysis below I really don't see any comments
about what is wrong but rather, how you had anticipated it would
be done. I've tried to answer the points in your email as best I
can, but the main issue is that it seemed to me that your proposal
was for a new version of SLF4J whereas I am trying to make the
changes without the need for a major release or breaking any
compatibility.
I assumed you tried to do the Logback counterpart of my SLF4J
redesign proposal - which wouldn't break compatibility but
introduces an additional artifact (tentatively called slf4j-n-api).
Instead of depending on slf4j-api, Logback classic would instead
depend on slf4j-n-api (which, in turn, introduces the slf4j-api
dependency for compatibility reasons).
I tried to implement your proposal to some degree. The most
important part to me was introducing the Message concept. For
minimal impact on users I did not want to introduce or change any
dependencies, other than requiring version changes.
On Jan 25, 2010, at 3:40 AM, Joern Huxhorn wrote:
Ok, I've had time to take a look now.
My plan was that the logback-classic Logger would simply extend
org.slf4j.n.helper.AbstractLoggerBase.
I saw that in your original note. That class doesn't currently
exist. I assume you would have created it. As I recall I thought
about that but decided against it. Logback has a lot of logic in
its Logger class and implementing AbstractLoggerBase in SLF4J
doesn't save much as you might think. In addition, all the
current Loggers implement the interface. Creating a new base class
that includes the Message API and then having Logback extend that
creates serious problems
But it does exist:
http://github.com/huxi/slf4j/blob/slf4j-redesign/slf4j-n-api/src/main/java/org/slf4j/n/helpers/AbstractLoggerBase.java
Is it possible that you did not look at the slf4j-redesign branch
but master?
Sorry. It has been quite a while since I wrote the code. Yes, I
looked at your code. IIRC the reason I didn't use it was that I
don't like calling the isEnabled methods that way. The log method is
going to result it Logback calling filterAndLog which results in
filtering being performed twice for each call. If the log method
doesn't call filterAndLog then the behavior of the log method would
be different than how Logback currently behaves. Of course, not
calling isEnabled would result in creating ParameterizedMessages
when no logging will occur. My implementation doesn't have this
problem.
Good point.
I've changed AbstractLoggerBase a bit.
It now directly implements log(level, marker, message, throwable) and
defines
protected abstract void performLogging(level, marker, message,
throwable)
An implementation is expected to not call isEnabled before performing
additional filtering and logging.
What I don't know right now:
Is it possible to reenable logging that would be disabled due to level
by using turbo-filters?
This wouldn't work anymore using my AbstractLoggerBase.
(which doesn't mean that the rest of the concept doesn't work)
Calling isEnabled was indeed meant to prevent creation of messages
that won't be logged.
This could probably be prevented by another method like
protected abstract boolean isEnabled(level, marker, messagePattern,
arguments)
that implemented the filtering logic.
I've never used TurboFilters so I tend to forget about their powers.
One problem of your implementation that I've found in the meantime is
that you are not using the Throwable that might have been detected by
ParameterizedMessage.
The logic looks like this:
public void log(Level level, Marker marker, Message message) {
if (!isEnabled(level, marker)) return;
if (message instanceof ParameterizedMessage) {
performLogging(level, marker, message, ((ParameterizedMessage)
message).getThrowable());
}
else {
performLogging(level, marker, message, null);
}
}
This is necessary to use both varargs and Throwables at the same time.
Assuming that TurboFilters can act on the optional Throwables, the
Message needs to be created before filtering.
Assuming the filters can NOT override a level, checking for level and
marker using isEnabled(level, marker) makes sense before filtering to
prevent the Message creation.
I really don't know.
The Logback classic Logger isn't extending anything at the moment
and the AbstractLoggerBase is simply implementing everything that
will likely stay the same for all Logger implementations.
It also supports proper deserialization of Logger instances already.
It's not necessary to extend it but it's certainly easier to do so
and doing so wouldn't introduce any problems in case of the Logback
classic Logger. It would simply remove a lot of boilerplate code
from the Logger implementation.
That would be true if my comment above wasn't true.
b) no new SLF4J version is required because c) backward
compatibility must be maintained.
That's the whole point. By leaving the original API untouched and
adding an additional one we'd have the possibility to change the
API without constraints.
My constraining goal was compiletime compatibility, but not binary
compatibility.
Binary compatibility simply can't be achieved without sacrificing
JDK<1.5 compatibility in SLF4J API - and I think that would be a
bad idea.
So I aimed for the next-best thing: compiletime compatibility
requiring only a change of the Logger and LoggerFactory import.
??? My code should be maintaining binary compatibility. Applications
written to older versions of SLF4J can drop in the new jars with no
change. New code can use the new API. They can coexist happily.
With my concept it stays binary compatible for the old API, i.e.
org.slf4j.Logger and org.slf4j.LoggerFactory.
Any code built against those would simply work but couldn't use the
Message logging and varargs since the original SLF4J API is frozen and
JDK14.
If you want Varargs (incl. Throwable evaluation) & Message you'd use
org.slf4j.n.Logger and org.slf4j.n.LoggerFactory.
The n-API is Java 5, the original one isn't.
If I was not working with those constraints I certainly would have
made a few different choices, but then I'm sure I would have had a
snowballs chance in hell of getting it adopted since it would
break so much stuff.
It wouldn't break anything. I tested it.
I won't believe that until I see a fork of Logback that depends on
your SLF4J implementation. It was very difficult to get Logback to
accept the Message and deal with it correctly.
Well, you did the hardest part of the work already, i.e. the Message
handling throughout the rest of the API.
I'd honestly like to have some input from Ceki because I don't want to
invest too much time if all of this ends in a "nope".
If you can find any breakage please let me know but I'm quite
confident that I didn't miss any point this time around.
For example:
during the evolution of SLF4J bug #31 I suggested that n.Logger
should extend Logger.
This would have resulted in breakage since it wouldn't have been
clear, during deserialization, what is expected by the
deserializing code. readResolve() needs to execute
LoggerFactory.getLogger(loggerName). How should we decide about
wrapping or not without creating kludgy code? Wrapping it each and
every time if n.Logger wasn't implemented by the returned instance?
Nope, this would have been quite bad.
If you notice there really is no extra overhead since in my fork
Logback's getLogger() always returns a MessageLogger. It isn't
really wrapped. No kludgy code is needed. Since MessageLogger
extends Logger anyone calling LoggerFactory.getLogger() will work
just fine. The wrapper is only needed if a Logger implementation
returns a Logger. Then MessageLoggerFactory will wrap it.
What MessageLoggerFactory? Now I'm lost. ;)
Joern.
_______________________________________________
logback-dev mailing list
logback-dev@qos.ch
http://qos.ch/mailman/listinfo/logback-dev
