You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4j-dev@logging.apache.org by ca...@apache.org on 2010/05/26 06:04:28 UTC
svn commit: r948306 - in
/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers: log4j2-api/
log4j2-api/src/main/java/org/apache/logging/log4j/
log4j2-api/src/main/java/org/apache/logging/log4j/message/
log4j2-api/src/main/java/org/apache/logging/log4j...
Author: carnold
Date: Wed May 26 04:04:27 2010
New Revision: 948306
URL: http://svn.apache.org/viewvc?rev=948306&view=rev
Log:
Code review comments using @issue, @doubt and @compare
Modified:
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/pom.xml
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Level.java
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Logger.java
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/MDC.java
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Marker.java
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/NDC.java
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/message/Message.java
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/ (props changed)
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/pom.xml
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Appender.java
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Filter.java
logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Layout.java
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/pom.xml
URL: http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/pom.xml?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/pom.xml (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/pom.xml Wed May 26 04:04:27 2010
@@ -28,6 +28,34 @@
<packaging>jar</packaging>
<name>Log4J API</name>
<description>The Log4J API</description>
+ <reporting>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-javadoc-plugin</artifactId>
+ <version>2.7</version>
+ <configuration>
+ <tags>
+ <tag>
+ <name>issue</name>
+ <placement>a</placement>
+ <head>JIRA issue:</head>
+ </tag>
+ <tag>
+ <name>doubt</name>
+ <placement>a</placement>
+ <head>Troublesome:</head>
+ </tag>
+ <tag>
+ <name>compare</name>
+ <placement>a</placement>
+ <head>Compare with:</head>
+ </tag>
+ </tags>
+ </configuration>
+ </plugin>
+ </plugins>
+ </reporting>
<dependencies>
<dependency>
<groupId>junit</groupId>
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Level.java
URL: http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Level.java?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Level.java (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Level.java Wed May 26 04:04:27 2010
@@ -29,6 +29,8 @@ package org.apache.logging.log4j;
* Typically, configuring a level in a filter or on a logger will cause logging events of that level and those
* that are more specific to pass through the filter.
* A special level, ALL, is guaranteed to capture all levels when used in logging configurations.
+ * @doubt There is not intermediate values available between WARN and INFO for example. Any reason why the existing log4j values were not retained?
+ * @doubt separating the converter from the type would allow alternative converters for different locales or different logging API's (for example, the same level could be FINER with one converter and TRACE with another.
*/
public enum Level {
OFF(0), FATAL(1), ERROR(2), WARN(3), INFO(4), DEBUG(5), TRACE(6), ALL(Integer.MAX_VALUE);
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Logger.java
URL: http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Logger.java?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Logger.java (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Logger.java Wed May 26 04:04:27 2010
@@ -19,8 +19,11 @@ package org.apache.logging.log4j;
import org.apache.logging.log4j.message.Message;
/**
- * This is the central class in the log4j package. Most logging
- * operations, except configuration, are done through this class.
+ * This is the central interface in the log4j package. Most logging
+ * operations, except configuration, are done through this interface.
+ * @doubt interface so complicated indepenent implementation unlikely. Should be refactored. LogMF/LogSF is a start as it moves message formatting out of logger.
+ * @doubt I'd prefer an abstract user context object over Marker. You could still use a Marker as your user context object,
+ * but you could also use an HttpServletRequest or any other context object.
*/
public interface Logger {
@@ -35,6 +38,8 @@ public interface Logger {
/**
* Log entry to a method.
* @param params The parameters to the method.
+ * @doubt Use of varargs results in array creation which can be a substantial portion of no-op case.
+ * LogMF/LogSF provides several overrides to avoid vararg except in edge cases.
*/
void entry(Object... params);
@@ -295,6 +300,7 @@ public interface Logger {
* Log a message with parameters at the <code>INFO</code> level.
* @param message the message to log.
* @param params parameters to the message.
+ * @doubt Likely to misinterpret existing log4j client code that intended to call info(Object,Throwable). Incurs array creation expense on every call.
*/
void info(String message, Object... params);
@@ -383,6 +389,7 @@ public interface Logger {
* Log a message with parameters at the <code>WARN</code> level.
* @param message the message to log.
* @param params parameters to the message.
+ * @doubt Likely to misinterpret existing log4j client code that intended to call info(Object,Throwable). Incurs array creation expense on every call.
*/
void warn(String message, Object... params);
@@ -471,6 +478,7 @@ public interface Logger {
* Log a message with parameters at the <code>ERROR</code> level.
* @param message the message to log.
* @param params parameters to the message.
+ * @doubt Likely to misinterpret existing log4j client code that intended to call info(Object,Throwable). Incurs array creation expense on every call.
*/
void error(String message, Object... params);
@@ -558,6 +566,7 @@ public interface Logger {
* Log a message with parameters at the <code>FATAL</code> level.
* @param message the message to log.
* @param params parameters to the message.
+ * @doubt Likely to misinterpret existing log4j client code that intended to call info(Object,Throwable). Incurs array creation expense on every call.
*/
void fatal(String message, Object... params);
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/MDC.java
URL: http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/MDC.java?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/MDC.java (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/MDC.java Wed May 26 04:04:27 2010
@@ -32,6 +32,8 @@ import java.util.Map;
* <p><b><em>The MDC is managed on a per thread basis</em></b>. A
* child thread automatically inherits a <em>copy</em> of the mapped
* diagnostic context of its parent.
+ *
+ * @doubt I'd throw the concept into a ThreadContext object.
*/
public final class MDC {
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Marker.java
URL: http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Marker.java?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Marker.java (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/Marker.java Wed May 26 04:04:27 2010
@@ -21,7 +21,6 @@ import java.util.concurrent.ConcurrentHa
import java.util.concurrent.ConcurrentMap;
/**
- *
*/
public class Marker implements Serializable {
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/NDC.java
URL: http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/NDC.java?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/NDC.java (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/NDC.java Wed May 26 04:04:27 2010
@@ -83,6 +83,7 @@ import java.util.concurrent.ConcurrentMa
* method. A thread may obtain a copy of its NDC with the {@link
* #cloneStack cloneStack} method and pass the reference to any other
* thread, in particular to a child.
+ * @doubt I'd combine MDC and NDC into a single ThreadContext object.
*/
public class NDC {
@@ -401,4 +402,4 @@ public class NDC {
}
}
}
-}
\ No newline at end of file
+}
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/message/Message.java
URL: http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/message/Message.java?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/message/Message.java (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/message/Message.java Wed May 26 04:04:27 2010
@@ -21,6 +21,7 @@ import java.util.Map;
/**
* An interface for various Message implementations that can be logged.
+ * @doubt Interfaces should rarely extend Serializable according to Effective Java 2nd Ed pg 291.
*/
public interface Message extends Serializable {
/**
@@ -34,6 +35,7 @@ public interface Message extends Seriali
* Returns the format portion of the Message
*
* @return The message format.
+ * @doubt Do all messages have a format? What syntax? Using a Formatter object could be cleaner.
*/
String getMessageFormat();
@@ -51,6 +53,7 @@ public interface Message extends Seriali
* provide values for. The Message must be able to return a formatted message even if
* no hints are provided.
* @return The Message hints.
+ * @doubt would seem to go better into a formatter or format object.
*/
Map<MessageHint, String> getHints();
}
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java
URL: http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java Wed May 26 04:04:27 2010
@@ -25,7 +25,8 @@ import org.apache.logging.log4j.message.
import org.apache.logging.log4j.message.SimpleMessage;
/**
- *
+ * @doubt See comments on Logger, interface is so complex that unlikely to be independently implemented.
+ * LogMF/LogSF separates those concerns out of Logger.
*/
public abstract class AbstractLogger implements Logger {
Propchange: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/
------------------------------------------------------------------------------
--- svn:ignore (added)
+++ svn:ignore Wed May 26 04:04:27 2010
@@ -0,0 +1 @@
+target
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/pom.xml
URL: http://svn.apache.org/viewvc/logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/pom.xml?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/pom.xml (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/pom.xml Wed May 26 04:04:27 2010
@@ -29,6 +29,35 @@
<name>Enhanced Pattern Layout Experiments</name>
<description>Experiments on Pattern Layout.</description>
<url>http://logging.apache.org/log4j/experimental</url>
+ <reporting>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-javadoc-plugin</artifactId>
+ <version>2.7</version>
+ <configuration>
+ <tags>
+ <tag>
+ <name>issue</name>
+ <placement>a</placement>
+ <head>JIRA issue:</head>
+ </tag>
+ <tag>
+ <name>doubt</name>
+ <placement>a</placement>
+ <head>Troublesome:</head>
+ </tag>
+ <tag>
+ <name>compare</name>
+ <placement>a</placement>
+ <head>Compare with:</head>
+ </tag>
+ </tags>
+ </configuration>
+ </plugin>
+ </plugins>
+ </reporting>
+
<dependencies>
<dependency>
<groupId>junit</groupId>
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Appender.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.java?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Appender.java (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Appender.java Wed May 26 04:04:27 2010
@@ -19,7 +19,7 @@ package org.apache.logging.log4j.core;
import java.util.List;
/**
- * @bug LOG4J2-36: Appender interface should be refactored
+ * @issue LOG4J2-36: Appender interface should be refactored
*/
public interface Appender {
/**
@@ -78,7 +78,7 @@ public interface Appender {
* Returns this appenders layout.
*
* @return the Layout for the Appender or null if none is configured.
- * @bug LOG4J2-36 Refactor into Channel
+ * @issue LOG4J2-36 Refactor into Channel
*/
Layout getLayout();
@@ -97,7 +97,7 @@ public interface Appender {
* the appender should return <code>true</code>.
*
* @return True if a Layout is required, false otherwise.
- * @bug LOG4J2-36 Refactor into Channel
+ * @issue LOG4J2-36 Refactor into Channel
*/
boolean requiresLayout();
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Filter.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.java?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Filter.java (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Filter.java Wed May 26 04:04:27 2010
@@ -13,10 +13,19 @@ public interface Filter {
ACCEPT, NEUTRAL, DENY
}
+ /**
+ * @doubt lifecycle management should be orthogonal.
+ */
void start();
+ /**
+ * @doubt lifecycle management should be orthogonal.
+ */
void stop();
+ /**
+ * @doubt lifecycle management should be orthogonal.
+ */
boolean isStarted();
Result getOnMismatch();
Modified: logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Layout.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/Layout.java?rev=948306&r1=948305&r2=948306&view=diff
==============================================================================
--- logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Layout.java (original)
+++ logging/log4j/branches/BRANCH_2_0_EXPERIMENTAL/rgoers/log4j2-core/src/main/java/org/apache/logging/log4j/core/Layout.java Wed May 26 04:04:27 2010
@@ -1,30 +1,37 @@
package org.apache.logging.log4j.core;
/**
- *
+ * @doubt There is still a need for a character-based layout for character based event sinks (databases, etc).
+ * Would introduce an EventEncoder, EventRenderer or something similar for the logging event to byte encoding.
*/
public interface Layout {
// Note that the line.separator property can be looked up even by
// applets.
+ /**
+ * @doubt It is very conceivable that distinct layouts might use distinct line separators. Should not be on the interface.
+ */
public static final String LINE_SEP = System.getProperty("line.separator");
public static final int LINE_SEP_LEN = LINE_SEP.length();
/**
* Formats the event suitable for display.
* @param event The Logging Event.
- * @return The formatted event String.
+ * @return The formatted event.
+ * @doubt Likely better to write to a OutputStream instead of return a byte[].
*/
byte[] format(LogEvent event);
/**
* Returns the header for the layout format.
* @return The header.
+ * @doubt the concept of header and footer is not universal, should not be on the base interface.
*/
byte[] getHeader();
/**
* Returns the format for the layout format.
* @return The footer.
+ * @doubt the concept of header and footer is not universal, should not be on the base interface.
*/
byte[] getFooter();
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org