vy commented on a change in pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#discussion_r744536353
##########
File path:
log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java
##########
@@ -623,7 +623,12 @@ public Configuration setConfiguration(final Configuration
config) {
map.putIfAbsent("hostName", "unknown");
}
map.putIfAbsent("contextName", contextName);
- config.start();
+ try {
+ config.start();
+ } catch (Exception e) {
+ config.stop();
+ return configuration;
+ }
Review comment:
We are swallowing the exception here. Shouldn't we check if it matches
first and otherwise raise it?
##########
File path: log4j-core/src/test/resources/log4j2-3182-error.xml
##########
@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="utf-8"?>
+<configuration status="WARN" packages="org.apache.logging.log4j.core.pattern"
monitorInterval="3">
+ <properties>
+ <property name="PATTERN">%d{yyyy-MM-dd HH:mm:ss.SSS} |-%-5level
[%thread] %c [%L] -| %msg%n</property>
+ </properties>
+
+ <appenders>
+ <Console name="CONSOLE" target="system_out">
+ <PatternLayout pattern="${PATTERN}" />
+ </Console>
+
+ <Kafka name="KafkaLog" topic="acm">
+ <PatternLayout pattern="%msg"/>
+ <Property name="bootstrap.servers">10.20.10.111111:9092</Property>
Review comment:
Mind documenting the reason for the deliberately incorrectly typed IP
address, please?
##########
File path:
log4j-core/src/test/java/org/apache/logging/log4j/core/config/LoggerContextChangeTest.java
##########
@@ -0,0 +1,69 @@
+package org.apache.logging.log4j.core.config;
Review comment:
License header is missing.
##########
File path: log4j-core/src/test/resources/log4j2-3182.xml
##########
@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="utf-8"?>
+<configuration status="WARN" packages="org.apache.logging.log4j.core.pattern"
monitorInterval="3">
+ <properties>
+ <property name="PATTERN">%d{yyyy-MM-dd HH:mm:ss.SSS} |-%-5level
[%thread] %c [%L] -| %msg%n</property>
+ </properties>
+
+ <appenders>
+ <Console name="CONSOLE" target="system_out">
+ <PatternLayout pattern="${PATTERN}" />
+ </Console>
+
+ <Kafka name="KafkaLog" topic="acm">
+ <PatternLayout pattern="%msg"/>
+ <Property name="bootstrap.servers">10.20.10.11:9092</Property>
Review comment:
I don't think this is an OS/network-independent solution. How do you
know that the Kafka server is on `10.20.10.11`? Further, doesn't this test
depend on a running Kafka server? If so, where do we start it?
##########
File path:
log4j-core/src/test/java/org/apache/logging/log4j/core/config/LoggerContextChangeTest.java
##########
@@ -0,0 +1,69 @@
+package org.apache.logging.log4j.core.config;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.junit.Assert;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import java.io.File;
+import java.util.Set;
+
+public class LoggerContextChangeTest {
+ private static String targetFileName = "log4j2-3182.xml";
+ private static String targetErrorFileName = "log4j2-3182-error.xml";
+ private static String tmpFileName = "log4j2-3182-tmp.xml";
+ private static String destDir = "target/test-classes/";
+
+ @BeforeAll
+ public static void beforeClass() {
+ System.setProperty("log4j2.configurationFile", "classpath:" +
targetFileName);
+ }
+
+
+ @Test
+ public void onChangeTest() {
+ String errorFileName = destDir + targetErrorFileName;
+ String originFileName = destDir + targetFileName;
+ String tmpFile = destDir + tmpFileName;
+ wait(10);
+ updateConfigFileModTime(originFileName, errorFileName, tmpFile);
+ wait(30);
Review comment:
Given that the current test suite takes more than 30 minutes, I am
reluctant to add a test that waits for 10+30=40 seconds.
--
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]
