vy commented on code in PR #2112:
URL: https://github.com/apache/logging-log4j2/pull/2112#discussion_r1432378086
##########
log4j-core-test/pom.xml:
##########
@@ -371,6 +375,32 @@
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>test-disruptor-4</id>
+ <goals>
+ <goal>test</goal>
+ </goals>
+ <configuration>
+ <additionalClasspathDependencies>
+ <dependency>
+ <groupId>com.lmax</groupId>
+ <artifactId>disruptor</artifactId>
+ <version>${disruptor4.version}</version>
+ </dependency>
+ </additionalClasspathDependencies>
+ <classpathDependencyExcludes>
+
<classpathDependencyExclude>com.lmax:disruptor</classpathDependencyExclude>
+ </classpathDependencyExcludes>
+
<groups>org.apache.logging.log4j.core.test.categories.AsyncLoggers</groups>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
Review Comment:
Sweet! I hope dependabot can take care of updating `disruptor4.version` in
this setting. Otherwise we can duplicate the dependency in the
`dependencyManagement` block of _this_ `pom.xml`.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/AsyncLoggerConfigDisruptor.java:
##########
@@ -155,6 +165,16 @@ private void notifyIntermediateProgress(final long
sequence) {
ringBufferElement.loggerConfig = loggerConfig;
};
+ private Log4jEventWrapperHandler createEventHandler() {
+ try {
+ return LoaderUtil.newInstanceOf(
+
"org.apache.logging.log4j.core.async.AsyncLoggerConfigDisruptor$Log4jEventWrapperHandler3");
+ } catch (final ReflectiveOperationException | LinkageError e) {
+ LOGGER.debug("LMAX Disruptor 3.x is missing, trying version 4.x.",
e);
+ }
+ return new Log4jEventWrapperHandler();
Review Comment:
1. Doesn't this imply that Disruptor 4 users will always get a debug message
telling the obvious?
2. Can't we rather `switch` on `DisruptorUtil.DISRUPTOR_MAJOR_VERSION` and
_only_ support 3 and 4?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/RingBufferLogEventHandler4.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.async;
+
+import com.lmax.disruptor.EventHandler;
+import com.lmax.disruptor.Sequence;
+import org.apache.logging.log4j.status.StatusLogger;
+import org.apache.logging.log4j.util.LoaderUtil;
+
+/**
+ * This event handler gets passed messages from the RingBuffer as they become
+ * available. Processing of these messages is done in a separate thread,
+ * controlled by the {@code Executor} passed to the {@code Disruptor}
+ * constructor.
+ */
+class RingBufferLogEventHandler4 implements EventHandler<RingBufferLogEvent> {
+
+ private static final int NOTIFY_PROGRESS_THRESHOLD = 50;
+ private Sequence sequenceCallback;
+ private int counter;
+ private long threadId = -1;
+
+ /**
+ * Returns the appropriate {@link EventHandler} for the version of LMAX
Disruptor used.
+ */
+ public static RingBufferLogEventHandler4 create() {
+ try {
+ return
LoaderUtil.newInstanceOf("org.apache.logging.log4j.core.async.RingBufferLogEventHandler");
+ } catch (final ReflectiveOperationException | LinkageError e) {
+ StatusLogger.getLogger().debug("LMAX Disruptor 3.x is missing,
trying version 4.x.", e);
+ }
+ return new RingBufferLogEventHandler4();
Review Comment:
My comment above for `AsyncLoggerConfigDisruptor.createEventHandler()`
applies here too.
##########
log4j-jpl/pom.xml:
##########
@@ -65,29 +65,26 @@
</dependency>
</dependencies>
- <profiles>
- <profile>
- <id>java8-tests</id>
- <activation>
- <property>
- <name>env.CI</name>
- <value>true</value>
- </property>
- </activation>
- <build>
- <plugins>
- <plugin>
- <groupId>org.apache.maven.plugins</groupId>
- <artifactId>maven-surefire-plugin</artifactId>
- <configuration combine.self="override">
- <reuseForks>false</reuseForks>
- <systemPropertyVariables>
- <java.awt.headless>true</java.awt.headless>
- </systemPropertyVariables>
- </configuration>
- </plugin>
- </plugins>
- </build>
- </profile>
- </profiles>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <executions>
+ <!-- Uses a different id than `default-test` to ignore the
`java8-tests` profile -->
+ <execution>
+ <id>run-tests</id>
+ <goals>
+ <goal>test</goal>
+ </goals>
+ </execution>
+ <execution>
+ <id>default-test</id>
+ <phase>none</phase>
Review Comment:
Why not `<configuration><skip>true` instead?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/RingBufferLogEventHandler.java:
##########
@@ -26,59 +25,12 @@
* controlled by the {@code Executor} passed to the {@code Disruptor}
* constructor.
*/
-public class RingBufferLogEventHandler implements
SequenceReportingEventHandler<RingBufferLogEvent>, LifecycleAware {
-
- private static final int NOTIFY_PROGRESS_THRESHOLD = 50;
- private Sequence sequenceCallback;
- private int counter;
- private long threadId = -1;
-
- @Override
- public void setSequenceCallback(final Sequence sequenceCallback) {
- this.sequenceCallback = sequenceCallback;
- }
-
- @Override
- public void onEvent(final RingBufferLogEvent event, final long sequence,
final boolean endOfBatch)
- throws Exception {
- try {
- // RingBufferLogEvents are populated by an EventTranslator. If an
exception is thrown during event
- // translation, the event may not be fully populated, but
Disruptor requires that the associated sequence
- // still be published since a slot has already been claimed in the
ring buffer. Ignore any such unpopulated
- // events. The exception that occurred during translation will
have already been propagated.
- if (event.isPopulated()) {
- event.execute(endOfBatch);
- }
- } finally {
- event.clear();
- // notify the BatchEventProcessor that the sequence has progressed.
- // Without this callback the sequence would not be progressed
- // until the batch has completely finished.
- notifyCallback(sequence);
- }
- }
-
- private void notifyCallback(final long sequence) {
- if (++counter > NOTIFY_PROGRESS_THRESHOLD) {
- sequenceCallback.set(sequence);
- counter = 0;
- }
- }
+public class RingBufferLogEventHandler extends RingBufferLogEventHandler4
+ implements SequenceReportingEventHandler<RingBufferLogEvent>,
LifecycleAware {
/**
- * Returns the thread ID of the background consumer thread, or {@code -1}
if the background thread has not started
- * yet.
- * @return the thread ID of the background consumer thread, or {@code -1}
+ * @deprecated Use the {@link RingBufferLogEventHandler4#create()} factory
method instead.
*/
- public long getThreadId() {
- return threadId;
- }
-
- @Override
- public void onStart() {
- threadId = Thread.currentThread().getId();
- }
-
- @Override
- public void onShutdown() {}
+ @Deprecated
+ public RingBufferLogEventHandler() {}
Review Comment:
No, AFAIC, we cannot point users to `RingBufferLogEventHandler4`, which is
an internal class. Could you instead add a `create()` method to this class,
delegate there to `RingBufferLogEventHandler4#create()`, and point users to
this public `create()`, please?
This deprecation doesn't need a mention in the changelog. But if you think
otherwise, go ahead and create one.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/AsyncLoggerDisruptor.java:
##########
@@ -122,7 +122,7 @@ public Thread newThread(final Runnable r) {
final ExceptionHandler<RingBufferLogEvent> errorHandler =
DisruptorUtil.getAsyncLoggerExceptionHandler();
disruptor.setDefaultExceptionHandler(errorHandler);
- final RingBufferLogEventHandler[] handlers = {new
RingBufferLogEventHandler()};
+ final RingBufferLogEventHandler4[] handlers =
{RingBufferLogEventHandler4.create()};
Review Comment:
I would rather point to `RBLEH.create()` I suggested below.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/DefaultAsyncWaitStrategyFactory.java:
##########
@@ -74,6 +74,19 @@ static WaitStrategy createDefaultWaitStrategy(final String
propertyName) {
LOGGER.trace(
"DefaultAsyncWaitStrategyFactory creating
TimeoutBlockingWaitStrategy(timeout={}, unit=MILLIS)",
timeoutMillis);
+ try {
+ // Check for the v 4.x version of the strategy, the version in 3.x
is not garbage-free.
+ if (DisruptorUtil.DISRUPTOR_MAJOR_VERSION == 4) {
Review Comment:
Nitpick: I would have preferred `if (...) { try {` rather than `try { if
(...) {`. The former makes it clear that the upcoming block is about Disruptor
4.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/async/RingBufferLogEventHandler4.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.async;
+
+import com.lmax.disruptor.EventHandler;
+import com.lmax.disruptor.Sequence;
+import org.apache.logging.log4j.status.StatusLogger;
+import org.apache.logging.log4j.util.LoaderUtil;
+
+/**
+ * This event handler gets passed messages from the RingBuffer as they become
+ * available. Processing of these messages is done in a separate thread,
+ * controlled by the {@code Executor} passed to the {@code Disruptor}
+ * constructor.
+ */
+class RingBufferLogEventHandler4 implements EventHandler<RingBufferLogEvent> {
Review Comment:
I am not able to understand the `4`-suffixed naming convention here. That
is, `RingBufferLogEventHandler4` creates the impression that it is supposed to
deliver some Disruptor4-specific functionality. On the contrary, it first
attempts to `create()` an instance using Disruptor3, and, on failure, falls
back to Disruptor4.
I would have preferred a more accurate and self-explanatory name (e.g.,
`RBLEHAdapter`) *and* documenting the fact that this class mediates an
`EventHandler<RingBufferLogEvent>` between multiple Disruptor major versions,
i.e., 3 and 4.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
