ppkarwasz commented on code in PR #3869:
URL: https://github.com/apache/logging-log4j2/pull/3869#discussion_r2280283598
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/LogEvent.java:
##########
@@ -48,9 +50,18 @@ public interface LogEvent {
* <p>
* Location information for both events might be computed by this
method.
* </p>
+ * <p>
+ * <strong>Warning:</strong> If {@code event.getMessage()} is an
instance of {@link ReusableMessage}, this method
+ * remove the parameter references from the original message. Callers
should:
+ * </p>
+ * <ol>
+ * <li>Either make sure that the {@code event} will not be used
again.</li>
+ * <li>Or call {@link LogEvent#toImmutable()} before calling this
method.</li>
+ * </ol>
+ * @since 3.0.0
*/
default LogEvent toMemento() {
- return new MementoLogEvent(this);
+ return new Log4jLogEvent.Builder(this).build();
Review Comment:
Isn't `toMemento()` a synonym of `toImmutable()` now? The suggestion of
calling `toImmutable()` before calling this method is misleading.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java:
##########
@@ -43,94 +43,117 @@
*/
public class Log4jLogEvent implements LogEvent {
- private final Marker marker;
+ // 1. Fields with an immutable type, initialized in the constructor
+ // Location data
+ private final String loggerFqcn;
private final Level level;
private final String loggerName;
- private Message message;
- private final MutableInstant instant = new MutableInstant();
+ private final Marker marker;
private final Throwable thrown;
- private final StringMap contextData;
- private final ThreadContext.ContextStack contextStack;
- private long threadId;
- private String threadName;
- private int threadPriority;
- private boolean endOfBatch = false;
/** @since Log4J 2.4 */
private final long nanoTime;
- // Location data
- private final String loggerFqcn;
- private @Nullable StackTraceElement source;
+ // This field is mutable, but its state is not shared with other objects.
+ private final MutableInstant instant = new MutableInstant();
+
+ // 2. Fields with setters, initialized in the constructor.
+ private boolean endOfBatch;
private boolean includeLocation;
+ // 3. Fields with an immutable type, initialized lazily.
+ // These fields self-initialize if not provided.
+ private @Nullable StackTraceElement source;
+ private String threadName;
+ private long threadId;
+ private int threadPriority;
+
+ // 4. Fields with a potentially mutable type.
+ // These fields can cause mutability problems for Log4jLogEvent.
+ private Message message;
+ private final StringMap contextData;
+ private final ThreadContext.ContextStack contextStack;
+
/** LogEvent Builder helper class. */
public static class Builder implements
org.apache.logging.log4j.plugins.util.Builder<LogEvent> {
+ // 1. Fields with an immutable type, initialized eagerly.
+ // These fields always keep the value assigned.
private String loggerFqcn;
- private Marker marker;
private Level level;
private String loggerName;
- private Message message;
+ private Marker marker;
private Throwable thrown;
+ private boolean endOfBatch;
+ private boolean includeLocation;
+ private long nanoTime;
+ // This field is mutable, but it is always copied.
private final MutableInstant instant = new MutableInstant();
- private StringMap contextData;
- private ThreadContext.ContextStack contextStack =
ThreadContext.getImmutableStack();
- private long threadId;
+
+ // 2. Fields with an immutable type, initialized lazily.
+ // These fields self-initialize if not provided.
+ private StackTraceElement source;
private String threadName;
+ private long threadId;
private int threadPriority;
- private StackTraceElement source;
- private boolean includeLocation;
- private boolean endOfBatch = false;
- private long nanoTime;
+
+ // 3. Fields with a mutable type.
+ // These fields require special handling.
+ private Message message;
+ private StringMap contextData;
+ private ThreadContext.ContextStack contextStack;
+
+ // 4. Fields with dependency-injected values.
private Clock clock;
private ContextDataInjector contextDataInjector;
public Builder() {
initDefaultContextData();
}
+ /**
+ * Initializes the builder with an <strong>immutable</strong> instance
or a copy of the log event fields.
+ *
+ * @param other The log event to copy.
+ */
public Builder(final LogEvent other) {
Review Comment:
_Nitpick_: instead of a public constructor, we could have a
`newBuilder(LogEvent)` factory method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
