I'm not sure what the appropriate way to respond to all the comments is going to be. Some of them I agree with and will make changes when I get the chance. Many of them just need an explanation or I disagree with the comment - not sure what to do about those. For example, in Logger -
@doubt All the isEnabled methods could be pushed into a filter interface. Not sure of the utility of having isEnabled * be able to examine the message pattern and parameters. 1. The Filter interface doesn't return a boolean but ACCEPT, DENY or NEUTRAL where isEnabled is true or false. Filters operate in a chained fashion until either ACCEPT or DENY is returned, so adding isEnabled to that interface doesn't make sense. 2. The value of having the the message and parameters is for performance. Creating the LogEvent isn't cheap. So it is essential to have global filters operate on the parameters directly. Of course, all the variations could be paired down but that would require user's to enter a null parameter where it isn't needed. I prefer to make the interface easier for the user, not the implementor. How should I add this to the @doubt? On May 26, 2010, at 8:26 PM, carn...@apache.org wrote: > Author: carnold > Date: Thu May 27 03:26:31 2010 > New Revision: 948664 > > URL: http://svn.apache.org/viewvc?rev=948664&view=rev > Log: > Additional code review comments > > Modified: > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/ErrorHandler.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Log4jLogEvent.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Logger.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LoggingException.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderBase.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderRuntimeException.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/ConsoleAppender.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/ListAppender.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamAppender.java > > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/filter/FilterBase.java > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/ErrorHandler.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/ErrorHandler.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/ErrorHandler.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/ErrorHandler.java > Thu May 27 03:26:31 2010 > @@ -20,6 +20,8 @@ import org.apache.logging.log4j.core.Log > > /** > * Appenders may delegate their error handling to <code>ErrorHandlers</code>. > + * @doubt if the appender interface is simplified, then error handling could > just be done by wrapping > + * a nested appender. > */ > public interface ErrorHandler { > > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Log4jLogEvent.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Log4jLogEvent.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Log4jLogEvent.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Log4jLogEvent.java > Thu May 27 03:26:31 2010 > @@ -101,14 +101,26 @@ public class Log4jLogEvent implements Lo > return throwable; > } > > + /** > + * @doubt Allows direct access to the map passed into the constructor, > would allow appender > + * or layout to manipulate event as seen by other appenders. > + */ > public Map<String, Object> getContextMap() { > return mdc; > } > > + /** > + * @doubt Allows direct access to the map passed into the constructor, > would allow appender > + * or layout to manipulate event as seen by other appenders. > + */ > public Stack<Object> getContextStack() { > return ndc; > } > > + /** > + * @doubt Not quite sure what is going on with the loop, but looks like > it might > + * drop only the deepest call from the fully qualified class, not > all of them. > + */ > public StackTraceElement getSource() { > if (fqcnOfLogger == null) { > return null; > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java > Thu May 27 03:26:31 2010 > @@ -39,6 +39,7 @@ public interface LogEvent { > /** > * Get thread name. > * @return thread name, may be null. > + * @doubt guess this could go into a thread context object too. > */ > String getThreadName(); > > @@ -61,6 +62,7 @@ public interface LogEvent { > * Get the MDC data; > * > * @return A copy of the Mapped Diagnostic Context or null. > + * @doubt as mentioned elsewhere, think MDC and NDC should be combined > into a thread context object. > */ > Map<String, Object> getContextMap(); > > @@ -68,6 +70,7 @@ public interface LogEvent { > * Get the NDC data; > * > * @return A copy of the Nested Diagnostic Context of null; > + * @doubt as mentioned elsewhere, think MDC and NDC should be combined > into a thread context object. > */ > Stack<Object> getContextStack(); > > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Logger.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Logger.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Logger.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Logger.java > Thu May 27 03:26:31 2010 > @@ -28,7 +28,8 @@ import java.util.List; > import java.util.Map; > > /** > - * > + * @doubt All the isEnabled methods could be pushed into a filter interface. > Not sure of the utility of having isEnabled > + * be able to examine the message pattern and parameters. > */ > public class Logger extends AbstractLogger { > //private static String FQCN = Logger.class.getName(); > @@ -164,12 +165,17 @@ public class Logger extends AbstractLogg > * volatile. > * > * @param config The new Configuration. > + * @doubt lost me on the comment, this.config is declared volatile. > */ > void updateConfiguration(Configuration config) { > this.config = new PrivateConfig(config, this); > } > > + /** > + * @doubt class is not immutable, so it should not be shared between > threads. > + */ > protected class PrivateConfig { > + /** @doubt public member variables? **/ > public final LoggerConfig loggerConfig; > public final Configuration config; > public Level level; > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java > Thu May 27 03:26:31 2010 > @@ -30,7 +30,7 @@ import java.util.concurrent.ConcurrentMa > */ > public class LoggerContext implements > org.apache.logging.log4j.spi.LoggerContext { > > - private ConcurrentMap<String, Logger> loggers = new > ConcurrentHashMap<String, Logger>(); > + private final ConcurrentMap<String, Logger> loggers = new > ConcurrentHashMap<String, Logger>(); > > private volatile Configuration config; > > @@ -73,6 +73,9 @@ public class LoggerContext implements or > config.removeFilter(filter); > } > > + /** > + * @doubt no check for null, could cause NPE if reconfigure is called. > + */ > public synchronized Configuration setConfiguration(Configuration config) { > Configuration prev = this.config; > this.config = config; > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LoggingException.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LoggingException.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LoggingException.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/LoggingException.java > Thu May 27 03:26:31 2010 > @@ -19,7 +19,7 @@ package org.apache.logging.log4j.core; > /** > * > * > - * > + * @doubt Unchecked? > * > */ > public class LoggingException extends RuntimeException { > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderBase.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderBase.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderBase.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderBase.java > Thu May 27 03:26:31 2010 > @@ -29,10 +29,13 @@ import java.util.List; > import java.util.concurrent.CopyOnWriteArrayList; > > /** > - * > + * @doubt Appender should be refactored as mentioned elsewhere > */ > public abstract class AppenderBase implements Appender { > > + /** > + * @doubt protected members? > + */ > protected boolean started = false; > > protected Layout layout = null; > @@ -56,10 +59,17 @@ public abstract class AppenderBase imple > return handler; > } > > + /** > + * @doubt no synchronization > + */ > public void setHandler(ErrorHandler handler) { > this.handler = handler; > } > > + > + /** > + * @doubt would be better to atomically replace a single Filter (which > could be composite) > + */ > public void addFilter(Filter filter) { > filters.add(filter); > } > @@ -84,6 +94,9 @@ public abstract class AppenderBase imple > return name; > } > > + /** > + * @doubt not synchronized. > + */ > public void setLayout(Layout layout) { > if (layout == null) { > handler.error("The layout for appender " + getName() + " cannot > be set to null"); > @@ -99,6 +112,10 @@ public abstract class AppenderBase imple > return false; > } > > + /** > + * @doubt I think it might be clearer just wrap an appender when you > want to swallow > + * exceptions, however you'd want the appender interface to be much > smaller to do that. > + */ > public boolean suppressException() { > return true; > } > @@ -121,6 +138,9 @@ public abstract class AppenderBase imple > return started; > } > > + /** > + * @doubt looks like some config code slipping into the core > + */ > public static Layout createLayout(Node node) { > Layout layout = null; > for (Node child : node.getChildren()) { > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderRuntimeException.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderRuntimeException.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderRuntimeException.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/AppenderRuntimeException.java > Thu May 27 03:26:31 2010 > @@ -17,7 +17,7 @@ > package org.apache.logging.log4j.core.appender; > > /** > - * > + * @doubt unchecked exception again > */ > public class AppenderRuntimeException extends RuntimeException { > > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/ConsoleAppender.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/ConsoleAppender.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/ConsoleAppender.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/ConsoleAppender.java > Thu May 27 03:26:31 2010 > @@ -27,6 +27,8 @@ import java.util.Map; > * ConsoleAppender appends log events to <code>System.out</code> or > * <code>System.err</code> using a layout specified by the user. The > * default target is <code>System.out</code>. > + * @doubt accessing System.out or .err as a byte stream instead of a writer > + * bypasses the JVM's knowledge of the proper encoding. > */ > @Plugin(name="Console",type="Core") > public class ConsoleAppender extends OutputStreamAppender { > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/ListAppender.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/ListAppender.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/ListAppender.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/ListAppender.java > Thu May 27 03:26:31 2010 > @@ -46,6 +46,8 @@ public class ListAppender extends Append > events.clear(); > } > > + /** @doubt think this caller would still see changes with no way > + to synchronize so they could get an consistent snapshot. */ > > public synchronized List<LogEvent> getEvents() { > return Collections.unmodifiableList(events); > } > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamAppender.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamAppender.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamAppender.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamAppender.java > Thu May 27 03:26:31 2010 > @@ -24,6 +24,12 @@ import java.io.OutputStream; > > /** > * > + * > + * @doubt This seems to be a cross between a character and byte-oriented > appender. > + * appenders would likely be either one or the other. > + * Would prefer to base on java.nio. Using an explicit > + * encoding might be expensive since it has to make an encoding > + * name to an encoder on every call. > */ > public abstract class OutputStreamAppender extends AppenderBase { > > @@ -306,4 +312,4 @@ public abstract class OutputStreamAppend > } > } > } > -} > \ No newline at end of file > +} > > Modified: > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/filter/FilterBase.java > URL: > http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/filter/FilterBase.java?rev=948664&r1=948663&r2=948664&view=diff > ============================================================================== > --- > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/filter/FilterBase.java > (original) > +++ > logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/filter/FilterBase.java > Thu May 27 03:26:31 2010 > @@ -29,6 +29,8 @@ import org.apache.logging.log4j.message. > * Users should extend this class to implement filters. Filters can be either > context wide or attached to > * an appender. A filter may choose to support being called only from the > context or only from an appender in > * which case it will only implement the required method(s). The rest will > default to return NEUTRAL. > + * > + * @doubt why extend FilterBase instead of implementing Filter. > */ > public abstract class FilterBase implements Filter { > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org > For additional commands, e-mail: log4j-dev-h...@logging.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org
