You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/10/29 02:47:35 UTC
[GitHub] [logging-log4j2] wuqian0808 opened a new pull request #593: LOG4J2-3182
wuqian0808 opened a new pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593
With MonitorInterval, if new config start failed, the previous watchmanager and scheduler will never shut down.
Besides, the async threads will increase.
This is for fixing this 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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #593: LOG4J2-3182 Protect KafkaManager against start failures upon config changes.
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#issuecomment-963786553
Open new clean pre instead
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #593: LOG4J2-3182 Protect KafkaManager against start failures upon config changes.
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#issuecomment-963903277
@wuqian0808, please don't abandon a PR like this. It becomes really difficult to keep track of what has happened and why.
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #593: LOG4J2-3182 Protect KafkaManager against start failures upon config changes.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#issuecomment-963417641
Hi all,
Did someone make sure the tests fail if the changes in main are not applied? I can't check ATM.
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #593: LOG4J2-3182
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#issuecomment-958764989
> This PR lacks a test case.
Test case added. Pls help to review and kindly help to advise
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a change in pull request #593: LOG4J2-3182
Posted by GitBox <gi...@apache.org>.
vy commented on a change in pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#discussion_r744533579
##########
File path: log4j-core/src/test/resources/log4j2-3182-error.xml
##########
@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="utf-8"?>
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"?>
Review comment:
License header is missing.
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #593: LOG4J2-3182
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#issuecomment-958764989
> This PR lacks a test case.
Test case added. Pls help to review and kindly help to advise
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on a change in pull request #593: LOG4J2-3182 Protect KafkaManager against start failures upon config changes.
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on a change in pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#discussion_r745262415
##########
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:
It doesn't depend on a running server. This is for mocking a thread leak during monitorinterval file on change suituation. Even the kafka ip address is not available, it won't affect this testcase
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #593: LOG4J2-3182
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#issuecomment-958764989
> This PR lacks a test case.
Test case added. Pls help to review and kindly help to advise
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #593: LOG4J2-3182
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#issuecomment-956138347
This PR lacks a test case.
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 closed pull request #593: LOG4J2-3182 Protect KafkaManager against start failures upon config changes.
Posted by GitBox <gi...@apache.org>.
wuqian0808 closed pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a change in pull request #593: LOG4J2-3182
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #593: LOG4J2-3182 Protect KafkaManager against start failures upon config changes.
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#issuecomment-962966682
@wuqian0808, mind updating the `changes.xml` too, please?
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on pull request #593: LOG4J2-3182
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#issuecomment-958764989
> This PR lacks a test case.
Test case added. Pls help to review and kindly help to advise
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] wuqian0808 commented on a change in pull request #593: LOG4J2-3182 Protect KafkaManager against start failures upon config changes.
Posted by GitBox <gi...@apache.org>.
wuqian0808 commented on a change in pull request #593:
URL: https://github.com/apache/logging-log4j2/pull/593#discussion_r745260747
##########
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:
I will change this part and raise the exception always
##########
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:
OK. I will reduce this wait time
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org