[GitHub] [logging-log4j2] vy commented on a change in pull request #428: LOG4J2-2916 Avoid redundant Kafka producer instantiation causing thread leaks.

[GitBox <gi...@apache.org>]

vy commented on a change in pull request #428:
URL: https://github.com/apache/logging-log4j2/pull/428#discussion_r522189283



##########
File path: log4j-kafka/src/test/java/org/apache/logging/log4j/kafka/appender/KafkaThreadTest.java
##########
@@ -0,0 +1,33 @@
+package org.apache.logging.log4j.kafka.appender;
+
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.junit.LoggerContextRule;
+import org.junit.Rule;
+import org.junit.Test;
+import java.util.Map;
+
+import static org.junit.Assert.assertTrue;
+
+public class KafkaThreadTest {
+
+    @Rule
+    public LoggerContextRule ctx = new LoggerContextRule("KafkaAppenderTest.xml");
+
+    @Test
+    public void testKafkaThreadLeak(){
+        LoggerContext context = ctx.getLoggerContext();
+        context.getContext(false).reconfigure();
+        context.getContext(false).reconfigure();
+        Map<Thread, StackTraceElement[]> maps = Thread.getAllStackTraces();
+        int kafkaThreadCount = 0;
+        if (maps != null && maps.size() >0) {
+            for (Thread thread : maps.keySet()) {
+                if (thread.getName().startsWith("kafka-producer")){
+                    kafkaThreadCount++;
+                }
+            }
+        }
+        assertTrue(kafkaThreadCount <= 5);
+    }

Review comment:
       Magical numbers in the tests, e.g., 5 here, have strong potential to bite us in the back later on. Would you mind revising this test as follows, please?
   
   ```java
   @Test
   void test_kafka_producer_threads_are_not_leaking_after_context_restart() {
   
       // Determine the initial number of threads.
       final LoggerContext context = ctx.getLoggerContext();
       final int initialThreadCount = kafkaProducerThreadCount();
   
       // Perform context restarts.
       final int contextRestartCount = 3;
       for (int i = 0; i < contextRestartCount; i++) {
           context.getContext(false).reconfigure();
       }
   
       // Verify the final thread count.
       final int lastThreadCount = kafkaProducerThreadCount();
       assertThat(lastThreadCount).equalsTo(initialThreadCount);
   
   }
   
   private static int kafkaProducerThreadCount() {
       // Return the number of threads prefixed with "kafka-producer".
   }
   ```
   Note that,
   - This is sort of a pseudo-code I jotted in the comment box, hence you need to fill it in yourself.
   - Try to use JUnit 5, if possible. (You can see examples in the source code.)




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org