Layout setter/getter methods in AppenderBase
Hello all, While working on chapter 11, migration of log4j appenders to logback-classic, it occured to me that the current way AppenderBase implements its layout setter and getter can be confusing. Here is the relevant code: abstract public class AppenderBase<E> extends ContextAwareBase implements Appender<E> { public Layout<E> getLayout() { return null; } public void setLayout(Layout<E> layout) { } // omitted code } The fact that the setLayout does nothing and getLayout always returns null is somethat unexpected and may suprise users. This has apparently already happended to Thilo Tanner as reported in LBCORE-56. See also http://jira.qos.ch/browse/LBCORE-56 I'd like to change AppenderBase to: abstract public class AppenderBase<E> extends ContextAwareBase implements Appender<E> { Layout<E> layout; public Layout<E> getLayout() { return layout; } public void setLayout(Layout<E> layout) { this.layout = layout; } // omitted code } The new code offers a reasonable default implementation for layout getter and setter methods. It should decrease the aforementioned surprise factor without causing harm to many appender implementations which do not need a layout. Comments? -- Ceki Gülcü Logback: The reliable, generic, fast and flexible logging framework for Java. http://logback.qos.ch
Hi Ceki, the only thing that I don't understand is why Appender requires a layout at all. It would by cleaner if there was a sub-interface, e.g. LayoutAwareAooender (just a spontaneous suggestion), that extended Appender and would add said methods. Some appenders, like SocketAppender or some fictitious appender that would simply serialize the events to a file (I'm planning to implement such an appender, btw), just don't need a layout at all. AppenderBase would then just implement the basic Appender interface, leaving the layout implementation to appenders that would really require it. This topic is covered by both http://jira.qos.ch/browse/LBCORE-1 and http://jira.qos.ch/browse/LBCORE-56 Regards, Joern. On 09.02.2009, at 19:50, Ceki Gulcu wrote:
Hello all,
While working on chapter 11, migration of log4j appenders to logback-classic, it occured to me that the current way AppenderBase implements its layout setter and getter can be confusing. Here is the relevant code:
abstract public class AppenderBase<E> extends ContextAwareBase implements Appender<E> {
public Layout<E> getLayout() { return null; }
public void setLayout(Layout<E> layout) { }
// omitted code }
The fact that the setLayout does nothing and getLayout always returns null is somethat unexpected and may suprise users. This has apparently already happended to Thilo Tanner as reported in LBCORE-56. See also http://jira.qos.ch/browse/LBCORE-56
I'd like to change AppenderBase to:
abstract public class AppenderBase<E> extends ContextAwareBase implements Appender<E> {
Layout<E> layout;
public Layout<E> getLayout() { return layout; }
public void setLayout(Layout<E> layout) { this.layout = layout; }
// omitted code }
The new code offers a reasonable default implementation for layout getter and setter methods. It should decrease the aforementioned surprise factor without causing harm to many appender implementations which do not need a layout.
Comments?
-- Ceki Gülcü Logback: The reliable, generic, fast and flexible logging framework for Java. http://logback.qos.ch _______________________________________________ logback-dev mailing list logback-dev@qos.ch http://qos.ch/mailman/listinfo/logback-dev
Hello Joern, At an earlier time, certain classes expected the Appender interface to have a setter/getter for the layout property. AbstractLayoutAction is one such class. However, it is no longer used. (I just removed it.) Many logback appenders do not require a layout and can have their layout property set to null. Having a layout property in Appender and AppenderBase is just noise for such appenders but otherwise not harmful. They have a property, i.e. layout, which they don't use... If the Appender interface did not contain the layout property, we would probably have two distinct appender base implementations, AppenderBase and AppenderBaseWithLayout (possibly with an additional interface such as AppenderWithLayout). Come to think of it, we would also need to handle the UnsyncronizedAppenderBase branch of the class hierarchy. Adding UnsyncronizedAppenderBaseWithLayout would be too unwieldy. :-) For developers coming from log4j, AppenderBase having the same implementation for setLayout and getLayout as log4j's AppenderSkeleton class would help them migrate to logback with a little bit more ease, at least with less surprise. I wonder what I was thinking when I implemented layout setter and getters in LayoutBase as nop. Joern Huxhorn wrote:
Hi Ceki,
the only thing that I don't understand is why Appender requires a layout at all. It would by cleaner if there was a sub-interface, e.g. LayoutAwareAooender (just a spontaneous suggestion), that extended Appender and would add said methods. Some appenders, like SocketAppender or some fictitious appender that would simply serialize the events to a file (I'm planning to implement such an appender, btw), just don't need a layout at all. AppenderBase would then just implement the basic Appender interface, leaving the layout implementation to appenders that would really require it.
This topic is covered by both http://jira.qos.ch/browse/LBCORE-1 and http://jira.qos.ch/browse/LBCORE-56
Regards, Joern.
-- Ceki Gülcü Logback: The reliable, generic, fast and flexible logging framework for Java. http://logback.qos.ch
On 09.02.2009, at 23:15, Ceki Gulcu wrote:
Hello Joern,
At an earlier time, certain classes expected the Appender interface to have a setter/getter for the layout property. AbstractLayoutAction is one such class. However, it is no longer used. (I just removed it.)
Oh, I see, "for historic reasons" :)
Many logback appenders do not require a layout and can have their layout property set to null. Having a layout property in Appender and AppenderBase is just noise for such appenders but otherwise not harmful. They have a property, i.e. layout, which they don't use...
Well, yes, you are right. It's not downright harmful... but it is somewhat confusing for users (not developers) of the appenders and makes them wonder what the layout is about. People actually asked me about that...
If the Appender interface did not contain the layout property, we would probably have two distinct appender base implementations, AppenderBase and AppenderBaseWithLayout (possibly with an additional interface such as AppenderWithLayout). Come to think of it, we would also need to handle the UnsyncronizedAppenderBase branch of the class hierarchy. Adding UnsyncronizedAppenderBaseWithLayout would be too unwieldy. :-)
I don't think that this would be necessary because it would be enough if the mentioned classes would simply implement Appender (without the layout methods) and appenders requiring the layout would additionally implement the extended interface, in just the way that is necessary for their desired behavior (e.g. even synchronized/locked if required, which isn't the case right now). This is more or less an aesthetic reasoning and I wouldn't consider it very important. Concerning your original question: the new implementation is definitely better than before!
For developers coming from log4j, AppenderBase having the same implementation for setLayout and getLayout as log4j's AppenderSkeleton class would help them migrate to logback with a little bit more ease, at least with less surprise.
I wonder what I was thinking when I implemented layout setter and getters in LayoutBase as nop.
I know this feeling all too well... But this is actually a good thing because it's a sign that we keep developing our skills ;) Joern.
Joern Huxhorn wrote:
Hi Ceki, the only thing that I don't understand is why Appender requires a layout at all. It would by cleaner if there was a sub-interface, e.g. LayoutAwareAooender (just a spontaneous suggestion), that extended Appender and would add said methods. Some appenders, like SocketAppender or some fictitious appender that would simply serialize the events to a file (I'm planning to implement such an appender, btw), just don't need a layout at all. AppenderBase would then just implement the basic Appender interface, leaving the layout implementation to appenders that would really require it. This topic is covered by both http://jira.qos.ch/browse/LBCORE-1 and http://jira.qos.ch/browse/LBCORE-56 Regards, Joern.
-- Ceki Gülcü Logback: The reliable, generic, fast and flexible logging framework for Java. http://logback.qos.ch _______________________________________________ logback-dev mailing list logback-dev@qos.ch http://qos.ch/mailman/listinfo/logback-dev
Hello, I agree with Joern, it would be cleaner to have a LayoutAware interface, and only appenders that use a Layout should implement it. The way it is now, people can set a layout on the SocketAppender, they don't get an exception, but the layout would never be used. I can understand the "historical" reasons, but IMO things like this can be changed as long as logback doesn't reach 1.0Maarten Related idea/proposal: an Encoder interface similar to Layout but returning a byte array instead of a String: public interface Encoder { byte[] encode(LoggingEvent event) } I recently worked on an AsyncSocketAppender (extending UnsyncronizedAppenderBase) and with this interface the wire-format would be pluggable. Some wire-formats I am thinking about: Apache Thrift, Google protobuf and of course Java serialization. I still have to implement these encoders and compare their perfomance. I will let you know when I get there. It would be really cool if we could define a wire-format based on Protobuf and/or Thrift that could also be used for encoding log4j events. But I guess it would be better to do this in a separate project ... regards, Maarten On Tue, Feb 10, 2009 at 1:01 AM, Joern Huxhorn <jhuxhorn@googlemail.com>wrote:
On 09.02.2009, at 23:15, Ceki Gulcu wrote:
Hello Joern,
At an earlier time, certain classes expected the Appender interface to have a setter/getter for the layout property. AbstractLayoutAction is one such class. However, it is no longer used. (I just removed it.)
Oh, I see, "for historic reasons" :)
Many logback appenders do not require a layout and can have their layout
property set to null. Having a layout property in Appender and AppenderBase is just noise for such appenders but otherwise not harmful. They have a property, i.e. layout, which they don't use...
Well, yes, you are right. It's not downright harmful... but it is somewhat confusing for users (not developers) of the appenders and makes them wonder what the layout is about. People actually asked me about that...
If the Appender interface did not contain the layout property, we would probably have two distinct appender base implementations, AppenderBase and AppenderBaseWithLayout (possibly with an additional interface such as AppenderWithLayout). Come to think of it, we would also need to handle the UnsyncronizedAppenderBase branch of the class hierarchy. Adding UnsyncronizedAppenderBaseWithLayout would be too unwieldy. :-)
I don't think that this would be necessary because it would be enough if the mentioned classes would simply implement Appender (without the layout methods) and appenders requiring the layout would additionally implement the extended interface, in just the way that is necessary for their desired behavior (e.g. even synchronized/locked if required, which isn't the case right now).
This is more or less an aesthetic reasoning and I wouldn't consider it very important.
Concerning your original question: the new implementation is definitely better than before!
For developers coming from log4j, AppenderBase having the same
implementation for setLayout and getLayout as log4j's AppenderSkeleton class would help them migrate to logback with a little bit more ease, at least with less surprise.
I wonder what I was thinking when I implemented layout setter and getters in LayoutBase as nop.
I know this feeling all too well... But this is actually a good thing because it's a sign that we keep developing our skills ;)
Joern.
Joern Huxhorn wrote:
Hi Ceki, the only thing that I don't understand is why Appender requires a layout at all. It would by cleaner if there was a sub-interface, e.g. LayoutAwareAooender (just a spontaneous suggestion), that extended Appender and would add said methods. Some appenders, like SocketAppender or some fictitious appender that would simply serialize the events to a file (I'm planning to implement such an appender, btw), just don't need a layout at all. AppenderBase would then just implement the basic Appender interface, leaving the layout implementation to appenders that would really require it. This topic is covered by both http://jira.qos.ch/browse/LBCORE-1 and http://jira.qos.ch/browse/LBCORE-56 Regards, Joern.
-- Ceki Gülcü Logback: The reliable, generic, fast and flexible logging framework for Java. http://logback.qos.ch _______________________________________________ logback-dev mailing list logback-dev@qos.ch http://qos.ch/mailman/listinfo/logback-dev
_______________________________________________ logback-dev mailing list logback-dev@qos.ch http://qos.ch/mailman/listinfo/logback-dev
Hi Maarteen, I really like the idea of pluggable encoders. Putting UnsyncronizedAppenderBase aside for a second, I could imagine the following class hierarchy: interface LayoutAware extends Appender; interface EncoderAware extends Appender; abstract class AppenderBase implements Appender; abstract class LayoutAwareAppenderBase ext. AppenderBase implements LayoutAware; abstract class EncodrAwareAppenderBase ext. AppenderBase implements LayoutAware; class WriterAppender extends LayoutAwareAppenderBase; class FileAppender extends WriterAppender; ... class SocketAppender extends EncoderAwareAppenderBase; Appenders such as DBAppender and SMTPAppender, where LoggingEvent to byte[] encoding nor layouts make sense, could extend AppenderBase directly. We should pursue this.... Maarten Bosteels wrote:
Hello,
I agree with Joern, it would be cleaner to have a LayoutAware interface, and only appenders that use a Layout should implement it. The way it is now, people can set a layout on the SocketAppender, they don't get an exception, but the layout would never be used.
I can understand the "historical" reasons, but IMO things like this can be changed as long as logback doesn't reach 1.0Maarten
Related idea/proposal: an Encoder interface similar to Layout but returning a byte array instead of a String:
public interface Encoder { byte[] encode(LoggingEvent event) }
I recently worked on an AsyncSocketAppender (extending UnsyncronizedAppenderBase) and with this interface the wire-format would be pluggable.
Some wire-formats I am thinking about: Apache Thrift, Google protobuf and of course Java serialization.
I still have to implement these encoders and compare their perfomance. I will let you know when I get there.
It would be really cool if we could define a wire-format based on Protobuf and/or Thrift that could also be used for encoding log4j events. But I guess it would be better to do this in a separate project ...
regards, Maarten
-- Ceki Gülcü Logback: The reliable, generic, fast and flexible logging framework for Java. http://logback.qos.ch
On Fri, Feb 13, 2009 at 3:25 PM, Ceki Gulcu <ceki@qos.ch> wrote:
Hi Maarteen,
I really like the idea of pluggable encoders.
Great.
Putting UnsyncronizedAppenderBase aside for a second, I could imagine the following class hierarchy:
interface LayoutAware extends Appender; interface EncoderAware extends Appender;
I don't understand. In my opinion LayoutAware and EncoderAware should be standalone interface with just a getter and a setter, and not extend Appender. All appenders that need a layout should implement LayoutAware. They can implement the interface themselves or - when its practical - inherit from LayoutAwareAppenderBase Why would you tie LayoutAware to the Appender interface. Imagine this Encoder implementation: it implements layoutAware but it's not an Appender public class LayoutBasedEncoder implements Encoder, LayoutAware { private Layout layout; public Layout getLayout() {...} public void setLayout(...) {...} public ByteBuffer encode(LoggingEvent event) { String str = layout.format(event); byte[] bytes = getUtf8Bytes(str); ByteBuffer buffer = ByteBuffer.allocate(bytes.length + 4); buffer.putInt(bytes.length); buffer.put(bytes); return buffer; } } PS: Not yet sure whether the encode method should return a byte[] or a java.nio.ByteBuffer, I think the latter is easier to use. regards, Maarten
abstract class AppenderBase implements Appender; abstract class LayoutAwareAppenderBase ext. AppenderBase implements LayoutAware; abstract class EncodrAwareAppenderBase ext. AppenderBase implements LayoutAware;
class WriterAppender extends LayoutAwareAppenderBase; class FileAppender extends WriterAppender; ... class SocketAppender extends EncoderAwareAppenderBase;
Appenders such as DBAppender and SMTPAppender, where LoggingEvent to byte[] encoding nor layouts make sense, could extend AppenderBase directly.
We should pursue this....
Maarten Bosteels wrote:
Hello,
I agree with Joern, it would be cleaner to have a LayoutAware interface, and only appenders that use a Layout should implement it. The way it is now, people can set a layout on the SocketAppender, they don't get an exception, but the layout would never be used.
I can understand the "historical" reasons, but IMO things like this can be changed as long as logback doesn't reach 1.0Maarten
Related idea/proposal: an Encoder interface similar to Layout but returning a byte array instead of a String:
public interface Encoder { byte[] encode(LoggingEvent event) }
I recently worked on an AsyncSocketAppender (extending UnsyncronizedAppenderBase) and with this interface the wire-format would be pluggable.
Some wire-formats I am thinking about: Apache Thrift, Google protobuf and of course Java serialization.
I still have to implement these encoders and compare their perfomance. I will let you know when I get there.
It would be really cool if we could define a wire-format based on Protobuf and/or Thrift that could also be used for encoding log4j events. But I guess it would be better to do this in a separate project ...
regards, Maarten
-- Ceki Gülcü Logback: The reliable, generic, fast and flexible logging framework for Java. http://logback.qos.ch _______________________________________________ logback-dev mailing list logback-dev@qos.ch http://qos.ch/mailman/listinfo/logback-dev
Maarten Bosteels wrote:
interface LayoutAware extends Appender; interface EncoderAware extends Appender;
I don't understand. In my opinion LayoutAware and EncoderAware should be standalone interface with just a getter and a setter, and not extend Appender.
Yes, obviously.
All appenders that need a layout should implement LayoutAware. They can implement the interface themselves or - when its practical - inherit from LayoutAwareAppenderBase
Agreed.
Why would you tie LayoutAware to the Appender interface.
You could tie the two interfaces by mistake. :-)
PS: Not yet sure whether the encode method should return a byte[] or a java.nio.ByteBuffer, I think the latter is easier to use.
That would would be an option to consider.
regards, Maarten -- Ceki Gülcü Logback: The reliable, generic, fast and flexible logging framework for Java. http://logback.qos.ch
I'm in favor of the two interfaces suggested but I wanted to point out that the current implementation of SocketAppender could not be changed in a compatible way to implement EncoderAware because it is currently using a stream of serialized objects instead of byte[] for each object (using a new ObjectOutputStream for each new object). This would have to be changed. On 13.02.2009, at 17:06, Ceki Gulcu wrote:
Maarten Bosteels wrote:
interface LayoutAware extends Appender; interface EncoderAware extends Appender; I don't understand. In my opinion LayoutAware and EncoderAware should be standalone interface with just a getter and a setter, and not extend Appender.
Yes, obviously.
All appenders that need a layout should implement LayoutAware. They can implement the interface themselves or - when its practical - inherit from LayoutAwareAppenderBase
Agreed.
Why would you tie LayoutAware to the Appender interface.
You could tie the two interfaces by mistake. :-)
PS: Not yet sure whether the encode method should return a byte[] or a java.nio.ByteBuffer, I think the latter is easier to use.
That would would be an option to consider.
I would suggest to use byte[] because it can easily be transformed into a ByteBuffer using ByteBuffer.wrap() while the other direction using byte[] ByteBuffer.buffer() is marked as an optional operation. Regards, Joern.
participants (3)
-
Ceki Gulcu -
Joern Huxhorn -
Maarten Bosteels