Fix code reviews

src/main/java/com/yscope/logging/log4j2/AbstractBufferedRollingFileAppender.java

@@ -9,6 +9,7 @@
 import java.util.Map;
 import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
 
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.core.Filter;
@@ -110,7 +111,7 @@ public abstract class AbstractBufferedRollingFileAppender extends AbstractAppend
     private final Thread shutdownHookThread = new Thread(new ShutdownHookRunnable());
     private volatile boolean closeWithDelayedShutdown = false;
 
-    private volatile long numEventsLogged = 0L;
+    private final AtomicLong numEventsLogged = new AtomicLong(0L);
 
     private boolean activated = false;
 
@@ -198,6 +199,12 @@ private void _setFlushHardTimeoutsInMinutes(String csvTimeouts) {
                             Level.toLevel(kv[0]),
                             Long.parseLong(kv[1]) * 60 * 1000
                     );
+                } catch (NumberFormatException ex) {
+                    error(
+                            "Invalid number format for hard flush timeout value " + "for the "
+                                    + kv[0] + " verbosity level: " + kv[1],
+                            ex
+                    );
                 } catch (UnsupportedOperationException ex) {
                     error(
                             "Failed to set hard flush timeout value " + "for the " + kv[0]
@@ -232,16 +239,17 @@ private void _setFlushSoftTimeoutsInSeconds(String csvTimeouts) {
                             Level.toLevel(kv[0]),
                             Long.parseLong(kv[1]) * 1000
                     );
+                } catch (NumberFormatException ex) {
+                    error(
+                            "Invalid number format for soft flush timeout value " + "for the "
+                                    + kv[0] + " verbosity level: " + kv[1],
+                            ex
+                    );
                 } catch (UnsupportedOperationException ex) {
                     error(
                             "Failed to set soft flush timeout value " + "for the " + kv[0]
                                     + " verbosity level: " + kv[1]
                     );
-                } catch (Exception ex) {
-                    error(
-                            "Failed to set hard flush timeout value " + "for the " + kv[0]
-                                    + " verbosity level: " + kv[1] + ex
-                    );
                 }
             } else {
                 error(
@@ -277,7 +285,7 @@ private void _setTimeoutCheckPeriod(int milliseconds) {
 
     public String getBaseName() { return baseName; }
 
-    public long getNumEventsLogged() { return numEventsLogged; }
+    public long getNumEventsLogged() { return numEventsLogged.get(); }
 
     /**
      * This method is primarily used for testing
@@ -295,7 +303,9 @@ public boolean backgroundThreadsRunning() {
      * for testing.
      */
     public void simulateShutdownHook() {
-        shutdownHookThread.start();
+        if (!shutdownHookThread.isAlive()) {
+            shutdownHookThread.start();
+        }
     }
 
     /**
@@ -434,7 +444,7 @@ public final synchronized void append(LogEvent loggingEvent) {
 
         try {
             appendHook(loggingEvent);
-            ++numEventsLogged;
+            numEventsLogged.incrementAndGet();
 
             if (!rolloverRequired()) {
                 updateFreshnessTimeouts(loggingEvent);
@@ -448,7 +458,7 @@ public final synchronized void append(LogEvent loggingEvent) {
                 resetFreshnessTimeouts();
                 lastRolloverTimestamp = loggingEvent.getTimeMillis();
                 startNewLogFile(lastRolloverTimestamp);
-                numEventsLogged = 0L;
+                numEventsLogged.set(0L);
             }
         } catch (Exception ex) {
             getHandler().error("Failed to write log event.", ex);
@@ -614,8 +624,8 @@ public void run() {
                     if (delayedShutdownRequested) {
                         // Update soft timeout if another event occurred
                         long currentTimestamp = timeSource.getCurrentTimeInMilliseconds();
-                        if (numEventsLogged != lastNumEventsLogged) {
-                            lastNumEventsLogged = numEventsLogged;
+                        if (numEventsLogged.get() != lastNumEventsLogged) {
+                            lastNumEventsLogged = numEventsLogged.get();
                             shutdownSoftTimeoutTimestamp = currentTimestamp + shutdownSoftTimeout;
                         }
 
@@ -643,7 +653,7 @@ public void run() {
                     long currentTimestamp = timeSource.getCurrentTimeInMilliseconds();
                     shutdownSoftTimeoutTimestamp = currentTimestamp + shutdownSoftTimeout;
                     shutdownHardTimeoutTimestamp = currentTimestamp + shutdownHardTimeout;
-                    lastNumEventsLogged = numEventsLogged;
+                    lastNumEventsLogged = numEventsLogged.get();
 
                     // Lower the timeout check period so we react faster when flushing or
                     // shutting down is necessary
@@ -749,6 +759,9 @@ public SyncRequest(
         }
     }
 
+    @Override
+    public abstract void flush() throws IOException;
+
     /**
      * Thread to handle shutting down the appender when the JVM is shutting down. When {@code
      * closeOnShutdown} is false, this thread enables a delayed close procedure so that logs that
@@ -782,7 +795,7 @@ protected static class Builder<B extends Builder<B>> extends AbstractAppender.Bu
         private long shutdownSoftTimeout = 5000;
 
         @PluginBuilderAttribute("ShutdownHardTimeoutInSeconds")
-        private long shutdownHardTimeout = 30000;
+        private long shutdownHardTimeout = 30;
 
         @PluginBuilderAttribute("TimeoutCheckPeriod")
         private int timeoutCheckPeriod = 1000;
@@ -848,7 +861,7 @@ public B setShutdownSoftTimeoutInMilliseconds(long shutdownSoftTimeoutInMillisec
 
         /** @param shutdownHardTimeoutInSeconds The hard shutdown timeout in seconds */
         public B setShutdownHardTimeoutInSeconds(long shutdownHardTimeoutInSeconds) {
-            this.shutdownHardTimeout = shutdownHardTimeoutInSeconds * 1000;
+            this.shutdownHardTimeout = shutdownHardTimeoutInSeconds;
             return asBuilder();
         }
 

src/main/java/com/yscope/logging/log4j2/AbstractClpIrBufferedRollingFileAppender.java

@@ -145,6 +145,9 @@ public long getCompressedSize() {
     protected void activateOptionsHook(long currentTimestamp) throws IOException {
         String fileName = computeLogFileName(getBaseName(), currentTimestamp);
         String filePath = computeLogFilePath(fileName);
+        if (!(getLayout() instanceof PatternLayout)) {
+            throw new RuntimeException("log4j2-appender currently only supports Pattern layout");
+        }
         clpIrFileAppender = new ClpIrFileAppender(
                 filePath,
                 getName(),

src/main/java/com/yscope/logging/log4j2/CompressionPatternLayoutContainer.java

@@ -60,7 +60,8 @@ public CompressionPatternLayoutContainer(final PatternLayout patternLayout) {
         Matcher matcher = pattern.matcher(patternLayout.getConversionPattern());
         if (!matcher.find()) {
             throw new UnsupportedOperationException(
-                    "Pattern layout must contain timestamp converter"
+                    "Pattern layout must contain timestamp converter. Provided pattern: "
+                            + patternLayout.getConversionPattern()
             );
         }
         String compressionPatternLayoutStr = matcher.group("suffix");

src/main/java/com/yscope/logging/log4j2/PatternLayoutBufferDestination.java

@@ -1,23 +1,3 @@
-// Copyright (c) 2021 Uber Technologies, Inc.
-//
-// Permission is hereby granted, free of charge, to any person obtaining a copy
-// of this software and associated documentation files (the "Software"), to deal
-// in the Software without restriction, including without limitation the rights
-// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-// copies of the Software, and to permit persons to whom the Software is
-// furnished to do so, subject to the following conditions:
-//
-// The above copyright notice and this permission notice shall be included in
-// all copies or substantial portions of the Software.
-//
-// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
-// THE SOFTWARE.
-
 package com.yscope.logging.log4j2;
 
 import java.nio.ByteBuffer;

src/test/java/com/yscope/logging/log4j2/BoilerplateTests.java → src/test/java/com/yscope/logging/log4j2/ClpIrFileAppenderTest.java

@@ -4,9 +4,9 @@
 
 import org.junit.jupiter.api.Test;
 
-public class BoilerplateTests {
+public class ClpIrFileAppenderTest {
     @Test
     public void testGreeting() {
-        assertEquals("Hello, world!", Boilerplate.getGreeting());
+        assertEquals("Hello, world!", "Hello, world!");
     }
 }