logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ralph Goers <ralph.go...@dslextreme.com>
Subject Re: svn commit: r948664 - in /logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core: ./ appender/ filter/
Date Thu, 27 May 2010 04:01:35 GMT
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, carnold@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-unsubscribe@logging.apache.org
> For additional commands, e-mail: log4j-dev-help@logging.apache.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org


Mime
View raw message