ppkarwasz commented on code in PR #1401:
URL: https://github.com/apache/logging-log4j2/pull/1401#discussion_r1393751655
##########
log4j-api-test/src/test/java/org/apache/logging/log4j/util/StringsTest.java:
##########
@@ -54,15 +54,6 @@ public void testEMPTY() {
assertEquals(0, Strings.EMPTY.length());
}
- @Test
- public void testConcat() {
- assertEquals("ab", Strings.concat("a", "b"));
- assertEquals("a", Strings.concat("a", ""));
- assertEquals("a", Strings.concat("a", null));
- assertEquals("b", Strings.concat("", "b"));
- assertEquals("b", Strings.concat(null, "b"));
- }
-
Review Comment:
We lost this test.
##########
log4j-1.2-api/src/main/java/org/apache/log4j/builders/layout/HtmlLayoutBuilder.java:
##########
@@ -78,6 +79,7 @@ public Layout parse(final PropertiesConfiguration config) {
private Layout createLayout(final String title, final boolean
locationInfo) {
return LayoutWrapper.adapt(HtmlLayout.newBuilder()
+ .setConfiguration(new DefaultConfiguration())
Review Comment:
We should use the `config` parameter of the `parse` methods instead.
There should be no `new DefaultConfiguration()` in any of the
`o.a.l.builders` classes.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/BlockingQueueFactory.java:
##########
@@ -23,7 +23,7 @@
*
* @since 2.7
*/
-public interface BlockingQueueFactory<E> {
+public interface BlockingQueueFactory {
Review Comment:
This interface should be removed in favor of `BoundedQueueFactory`.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/QueueFactory.java:
##########
@@ -14,18 +14,19 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.logging.log4j.layout.template.json.util;
+package org.apache.logging.log4j.util;
-import java.util.function.Consumer;
-import java.util.function.Supplier;
+import java.util.Queue;
+/**
+ * A {@link Queue} factory contract.
+ *
+ * @see QueueFactories
+ * @since 3.0.0
+ */
@FunctionalInterface
-public interface RecyclerFactory {
-
- default <V> Recycler<V> create(final Supplier<V> supplier) {
- return create(supplier, ignored -> {});
- }
+public interface QueueFactory {
- <V> Recycler<V> create(Supplier<V> supplier, Consumer<V> cleaner);
+ <E> Queue<E> create();
Review Comment:
Could we condense this with `BoundedQueueFactory` and `BlockingQueueFactory`
from `log4j-core`?
The `create` method should probably also require a type of queue (SPSC or
MPSC, I don't believe we need multi-consumer queues right now).
Since these factories might be in separate modules (retrieved by
`ServiceLoader`) and we can not use plugin annotation, a `getName()` method
might be necessary.
##########
log4j-api/pom.xml:
##########
@@ -55,5 +55,10 @@
<artifactId>org.osgi.resource</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.jctools</groupId>
+ <artifactId>jctools-core</artifactId>
+ <optional>true</optional>
+ </dependency>
Review Comment:
I would rather move the code that depend on it to a new module
(`log4j-queue-jctools`?).
Since we are in `log4j-api`, the code could detect the factory through
`ServiceLoader`.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java:
##########
@@ -91,12 +91,8 @@ public Builder() {
public Builder(final LogEvent other) {
Objects.requireNonNull(other);
- if (other instanceof RingBufferLogEvent) {
- ((RingBufferLogEvent) other).initializeBuilder(this);
- return;
- }
- if (other instanceof MutableLogEvent) {
- ((MutableLogEvent) other).initializeBuilder(this);
+ if (other instanceof ReusableLogEvent) {
+ ((ReusableLogEvent) other).initializeBuilder(this);
Review Comment:
It is nice to see those `instanceof` disappear, although it would be better
to have a centralized logic on how to initialize the builder.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/ReusableLogEvent.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.logging.log4j.core;
+
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.ThreadContext;
+import org.apache.logging.log4j.core.impl.Log4jLogEvent;
+import org.apache.logging.log4j.core.time.Instant;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.util.StringMap;
+
+public interface ReusableLogEvent extends LogEvent {
+ /**
+ * Clears all references this event has to other objects.
+ */
+ void clear();
+
+ void setLoggerFqcn(final String loggerFqcn);
+
+ void setMarker(final Marker marker);
+
+ void setLevel(final Level level);
+
+ void setLoggerName(final String loggerName);
+
+ void setMessage(final Message message);
+
+ void setThrown(final Throwable thrown);
+
+ void setTimeMillis(final long timeMillis);
+
+ void setInstant(final Instant instant);
+
+ void setSource(final StackTraceElement source);
+
+ @Override
+ StringMap getContextData();
+
+ void setContextData(final StringMap mutableContextData);
+
+ void setContextStack(final ThreadContext.ContextStack contextStack);
+
+ void setThreadId(final long threadId);
+
+ void setThreadName(final String threadName);
+
+ void setThreadPriority(final int threadPriority);
+
+ void setNanoTime(final long nanoTime);
+
+ /**
+ * Initializes the specified {@code Log4jLogEvent.Builder} from this
{@code ReusableLogEvent}.
+ * @param builder the builder whose fields to populate
+ */
+ void initializeBuilder(final Log4jLogEvent.Builder builder);
Review Comment:
I am a big fan of this interface: I would love to just have
`Message/ReusableMessage` and `LogEvent/ReusableLogEvent` and logic that
transfers data between them that **does not** depend on their concrete
implementation.
I understand that doing this is **hard**, because we have a lot of mutable
parts:
- location information may be absent and we need to retrieve it while
copying from one log event to another,
- all context data implementations implement `StringMap` and are mutable,
however sometimes we have a guarantee that the data will be delivered to an
appender **before** it is reset,
- sometimes we can just swap data between two reusable messages/events if we
know that we are the last user of that message/event.
I think that this interface merits its own Github issue.
--
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
