[GitHub] [flink] metaswirl commented on a diff in pull request #19272: [FLINK-26710] fix TestLoggerResource
metaswirl commented on code in PR #19272:
URL: https://github.com/apache/flink/pull/19272#discussion_r841709790
##
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/testutils/logging/TestLoggerResourceTest.java:
##
@@ -0,0 +1,119 @@
+/*
+ * 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.flink.testutils.logging;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.junit.jupiter.api.Test;
+import org.slf4j.event.Level;
+
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * {@link TestLoggerResourceTest} ensures that the use of {@link
TestLoggerResource} combined with
+ * other loggers (including multiple instances of itself) does not lead to
unexpected behavior.
+ */
+public class TestLoggerResourceTest {
+final String parentLoggerName = TestLoggerResourceTest.class.getName() +
".parent";
+final String childLoggerName = parentLoggerName + ".child";
+final Logger parentLogger = LogManager.getLogger(parentLoggerName);
+final Logger childLogger = LogManager.getLogger(childLoggerName);
+
+@Test
+public void loggerWithoutChild() throws Throwable {
+try (TestLoggerResource.SingleTestResource parentResource =
+TestLoggerResource.asSingleTestResource(parentLoggerName,
Level.INFO)) {
+parentLogger.info("child-info");
+parentLogger.debug("child-debug");
+List msgs = parentResource.getMessages();
+assertThat(msgs).containsExactly("child-info");
+}
+}
+
+@Test
+public void loggerIsAlreadyDefined() throws Throwable {
+// Sometimes a logger is already defined for the same class (e.g., to
debug the test on
+// failure.
+try (TestLoggerResource.SingleTestResource outerResource =
+
TestLoggerResource.asSingleTestResource(parentLoggerName, Level.INFO);
+TestLoggerResource.SingleTestResource innerResource =
+
TestLoggerResource.asSingleTestResource(parentLoggerName, Level.DEBUG)) {
+parentLogger.info("child-info");
+parentLogger.debug("child-debug");
+List msgsInner = innerResource.getMessages();
+assertThat(msgsInner).containsExactly("child-info", "child-debug");
+List msgsOuter = outerResource.getMessages();
+assertThat(msgsOuter).contains("child-info");
Review Comment:
ok
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] metaswirl commented on a diff in pull request #19272: [FLINK-26710] fix TestLoggerResource
metaswirl commented on code in PR #19272:
URL: https://github.com/apache/flink/pull/19272#discussion_r841709315
##
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/testutils/logging/TestLoggerResourceTest.java:
##
@@ -0,0 +1,119 @@
+/*
+ * 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.flink.testutils.logging;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.junit.jupiter.api.Test;
+import org.slf4j.event.Level;
+
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * {@link TestLoggerResourceTest} ensures that the use of {@link
TestLoggerResource} combined with
+ * other loggers (including multiple instances of itself) does not lead to
unexpected behavior.
+ */
+public class TestLoggerResourceTest {
+final String parentLoggerName = TestLoggerResourceTest.class.getName() +
".parent";
+final String childLoggerName = parentLoggerName + ".child";
+final Logger parentLogger = LogManager.getLogger(parentLoggerName);
+final Logger childLogger = LogManager.getLogger(childLoggerName);
+
+@Test
+public void loggerWithoutChild() throws Throwable {
+try (TestLoggerResource.SingleTestResource parentResource =
+TestLoggerResource.asSingleTestResource(parentLoggerName,
Level.INFO)) {
+parentLogger.info("child-info");
+parentLogger.debug("child-debug");
+List msgs = parentResource.getMessages();
+assertThat(msgs).containsExactly("child-info");
+}
+}
+
+@Test
+public void loggerIsAlreadyDefined() throws Throwable {
+// Sometimes a logger is already defined for the same class (e.g., to
debug the test on
+// failure.
+try (TestLoggerResource.SingleTestResource outerResource =
+
TestLoggerResource.asSingleTestResource(parentLoggerName, Level.INFO);
+TestLoggerResource.SingleTestResource innerResource =
+
TestLoggerResource.asSingleTestResource(parentLoggerName, Level.DEBUG)) {
+parentLogger.info("child-info");
+parentLogger.debug("child-debug");
Review Comment:
fixed
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] metaswirl commented on a diff in pull request #19272: [FLINK-26710] fix TestLoggerResource
metaswirl commented on code in PR #19272:
URL: https://github.com/apache/flink/pull/19272#discussion_r841708284
##
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/testutils/logging/TestLoggerResourceTest.java:
##
@@ -0,0 +1,119 @@
+/*
+ * 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.flink.testutils.logging;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.junit.jupiter.api.Test;
+import org.slf4j.event.Level;
+
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * {@link TestLoggerResourceTest} ensures that the use of {@link
TestLoggerResource} combined with
+ * other loggers (including multiple instances of itself) does not lead to
unexpected behavior.
+ */
+public class TestLoggerResourceTest {
+final String parentLoggerName = TestLoggerResourceTest.class.getName() +
".parent";
+final String childLoggerName = parentLoggerName + ".child";
+final Logger parentLogger = LogManager.getLogger(parentLoggerName);
+final Logger childLogger = LogManager.getLogger(childLoggerName);
Review Comment:
fixed
##
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/testutils/logging/TestLoggerResourceTest.java:
##
@@ -0,0 +1,119 @@
+/*
+ * 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.flink.testutils.logging;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.junit.jupiter.api.Test;
+import org.slf4j.event.Level;
+
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * {@link TestLoggerResourceTest} ensures that the use of {@link
TestLoggerResource} combined with
+ * other loggers (including multiple instances of itself) does not lead to
unexpected behavior.
+ */
+public class TestLoggerResourceTest {
Review Comment:
fixed
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] metaswirl commented on a diff in pull request #19272: [FLINK-26710] fix TestLoggerResource
metaswirl commented on code in PR #19272:
URL: https://github.com/apache/flink/pull/19272#discussion_r841677959
##
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/logging/TestLoggerResource.java:
##
@@ -43,52 +52,125 @@
private final String loggerName;
private final org.slf4j.event.Level level;
+@Nullable private LoggerConfig backupLoggerConfig = null;
private ConcurrentLinkedQueue loggingEvents;
public TestLoggerResource(Class clazz, org.slf4j.event.Level level) {
-this.loggerName = clazz.getCanonicalName();
+this(clazz.getCanonicalName(), level);
+}
+
+private TestLoggerResource(String loggerName, org.slf4j.event.Level level)
{
+this.loggerName = loggerName;
this.level = level;
}
public List getMessages() {
return new ArrayList<>(loggingEvents);
}
+private static String generateRandomString() {
+return UUID.randomUUID().toString().replace("-", "");
+}
+
@Override
protected void before() throws Throwable {
loggingEvents = new ConcurrentLinkedQueue<>();
+final LoggerConfig previousLoggerConfig =
+LOGGER_CONTEXT.getConfiguration().getLoggerConfig(loggerName);
+
+final Level previousLevel = previousLoggerConfig.getLevel();
+final Level userDefinedLevel = Level.getLevel(level.name());
+
+// Set log level to last specific. This ensures that the parent still
receives all log
+// lines.
+// WARN is more specific than INFO is more specific than DEBUG etc.
+final Level newLevel =
+userDefinedLevel.isMoreSpecificThan(previousLevel)
+? previousLevel
+: userDefinedLevel;
+
+// Filter log lines according to user requirements.
+final Filter levelFilter =
+ThresholdFilter.createFilter(
+userDefinedLevel, Filter.Result.ACCEPT,
Filter.Result.DENY);
+
Appender testAppender =
-new AbstractAppender("test-appender", null, null, false) {
+new AbstractAppender(
+"test-appender-" + generateRandomString(),
levelFilter, null, false) {
@Override
public void append(LogEvent event) {
loggingEvents.add(event.getMessage().getFormattedMessage());
}
};
testAppender.start();
-AppenderRef appenderRef =
AppenderRef.createAppenderRef(testAppender.getName(), null, null);
-LoggerConfig logger =
+LoggerConfig loggerConfig =
LoggerConfig.createLogger(
-false,
-Level.getLevel(level.name()),
-"test",
+true,
+newLevel,
+loggerName,
null,
-new AppenderRef[] {appenderRef},
+new AppenderRef[] {},
null,
LOGGER_CONTEXT.getConfiguration(),
null);
-logger.addAppender(testAppender, null, null);
+loggerConfig.addAppender(testAppender, null, null);
-LOGGER_CONTEXT.getConfiguration().addLogger(loggerName, logger);
+if (previousLoggerConfig.getName().equals(loggerName)) {
+// remove the previous logger config for the duration of the test
+backupLoggerConfig = previousLoggerConfig;
+LOGGER_CONTEXT.getConfiguration().removeLogger(loggerName);
Review Comment:
In this case the old logger will not be replaced. At least this is what I
see in my experiments. There is no clear documentation for this.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] metaswirl commented on a diff in pull request #19272: [FLINK-26710] fix TestLoggerResource
metaswirl commented on code in PR #19272:
URL: https://github.com/apache/flink/pull/19272#discussion_r841673414
##
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/logging/TestLoggerResource.java:
##
@@ -43,52 +52,125 @@
private final String loggerName;
private final org.slf4j.event.Level level;
+@Nullable private LoggerConfig backupLoggerConfig = null;
private ConcurrentLinkedQueue loggingEvents;
public TestLoggerResource(Class clazz, org.slf4j.event.Level level) {
-this.loggerName = clazz.getCanonicalName();
+this(clazz.getCanonicalName(), level);
+}
+
+private TestLoggerResource(String loggerName, org.slf4j.event.Level level)
{
+this.loggerName = loggerName;
this.level = level;
}
public List getMessages() {
return new ArrayList<>(loggingEvents);
}
+private static String generateRandomString() {
+return UUID.randomUUID().toString().replace("-", "");
+}
+
@Override
protected void before() throws Throwable {
loggingEvents = new ConcurrentLinkedQueue<>();
+final LoggerConfig previousLoggerConfig =
+LOGGER_CONTEXT.getConfiguration().getLoggerConfig(loggerName);
+
+final Level previousLevel = previousLoggerConfig.getLevel();
+final Level userDefinedLevel = Level.getLevel(level.name());
+
+// Set log level to last specific. This ensures that the parent still
receives all log
+// lines.
+// WARN is more specific than INFO is more specific than DEBUG etc.
+final Level newLevel =
+userDefinedLevel.isMoreSpecificThan(previousLevel)
+? previousLevel
+: userDefinedLevel;
+
+// Filter log lines according to user requirements.
+final Filter levelFilter =
+ThresholdFilter.createFilter(
+userDefinedLevel, Filter.Result.ACCEPT,
Filter.Result.DENY);
+
Appender testAppender =
-new AbstractAppender("test-appender", null, null, false) {
+new AbstractAppender(
+"test-appender-" + generateRandomString(),
levelFilter, null, false) {
@Override
public void append(LogEvent event) {
loggingEvents.add(event.getMessage().getFormattedMessage());
}
};
testAppender.start();
-AppenderRef appenderRef =
AppenderRef.createAppenderRef(testAppender.getName(), null, null);
-LoggerConfig logger =
+LoggerConfig loggerConfig =
LoggerConfig.createLogger(
-false,
-Level.getLevel(level.name()),
-"test",
+true,
+newLevel,
+loggerName,
null,
-new AppenderRef[] {appenderRef},
+new AppenderRef[] {},
null,
LOGGER_CONTEXT.getConfiguration(),
null);
-logger.addAppender(testAppender, null, null);
+loggerConfig.addAppender(testAppender, null, null);
-LOGGER_CONTEXT.getConfiguration().addLogger(loggerName, logger);
+if (previousLoggerConfig.getName().equals(loggerName)) {
+// remove the previous logger config for the duration of the test
+backupLoggerConfig = previousLoggerConfig;
+LOGGER_CONTEXT.getConfiguration().removeLogger(loggerName);
+
+// combine appender set
+// Note: The appender may still receive more or less messages
depending on the log level
+// difference between the two logger
+for (Appender appender :
previousLoggerConfig.getAppenders().values()) {
+loggerConfig.addAppender(appender, null, null);
+}
+}
+
+LOGGER_CONTEXT.getConfiguration().addLogger(loggerName, loggerConfig);
LOGGER_CONTEXT.updateLoggers();
}
@Override
protected void after() {
LOGGER_CONTEXT.getConfiguration().removeLogger(loggerName);
+if (backupLoggerConfig != null) {
Review Comment:
A very good point
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] metaswirl commented on a diff in pull request #19272: [FLINK-26710] fix TestLoggerResource
metaswirl commented on code in PR #19272:
URL: https://github.com/apache/flink/pull/19272#discussion_r841670907
##
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/logging/TestLoggerResource.java:
##
@@ -43,52 +52,125 @@
private final String loggerName;
private final org.slf4j.event.Level level;
+@Nullable private LoggerConfig backupLoggerConfig = null;
private ConcurrentLinkedQueue loggingEvents;
public TestLoggerResource(Class clazz, org.slf4j.event.Level level) {
-this.loggerName = clazz.getCanonicalName();
+this(clazz.getCanonicalName(), level);
+}
+
+private TestLoggerResource(String loggerName, org.slf4j.event.Level level)
{
+this.loggerName = loggerName;
this.level = level;
}
public List getMessages() {
return new ArrayList<>(loggingEvents);
}
+private static String generateRandomString() {
+return UUID.randomUUID().toString().replace("-", "");
+}
+
@Override
protected void before() throws Throwable {
loggingEvents = new ConcurrentLinkedQueue<>();
+final LoggerConfig previousLoggerConfig =
+LOGGER_CONTEXT.getConfiguration().getLoggerConfig(loggerName);
+
+final Level previousLevel = previousLoggerConfig.getLevel();
+final Level userDefinedLevel = Level.getLevel(level.name());
+
+// Set log level to last specific. This ensures that the parent still
receives all log
+// lines.
+// WARN is more specific than INFO is more specific than DEBUG etc.
+final Level newLevel =
+userDefinedLevel.isMoreSpecificThan(previousLevel)
+? previousLevel
+: userDefinedLevel;
+
+// Filter log lines according to user requirements.
+final Filter levelFilter =
+ThresholdFilter.createFilter(
+userDefinedLevel, Filter.Result.ACCEPT,
Filter.Result.DENY);
+
Appender testAppender =
-new AbstractAppender("test-appender", null, null, false) {
+new AbstractAppender(
+"test-appender-" + generateRandomString(),
levelFilter, null, false) {
@Override
public void append(LogEvent event) {
loggingEvents.add(event.getMessage().getFormattedMessage());
}
};
testAppender.start();
-AppenderRef appenderRef =
AppenderRef.createAppenderRef(testAppender.getName(), null, null);
-LoggerConfig logger =
+LoggerConfig loggerConfig =
LoggerConfig.createLogger(
-false,
-Level.getLevel(level.name()),
-"test",
+true,
Review Comment:
If you disable additivity, then the lines are not forwarded to the parent
logger. The consequence is that the lines disappear for the user.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
