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: [email protected]
For additional commands, e-mail: [email protected]
