chia7712 commented on code in PR #18185:
URL: https://github.com/apache/kafka/pull/18185#discussion_r1898188108
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -127,14 +127,23 @@ public synchronized Map<String, LoggerLevel> allLevels() {
public synchronized List<String> setLevel(String namespace, Level level) {
Objects.requireNonNull(namespace, "Logging namespace may not be null");
Objects.requireNonNull(level, "Level may not be null");
+ String internalNameSpace = isValidRootLoggerName(namespace) ?
LogManager.ROOT_LOGGER_NAME : namespace;
+
+ log.info("Setting level of namespace {} and children to {}",
internalNameSpace, level);
- log.info("Setting level of namespace {} and children to {}",
namespace, level);
- List<org.apache.logging.log4j.Logger> childLoggers =
loggers(namespace);
+ var loggers = loggers(internalNameSpace);
+ var nameToLevel = allLevels();
List<String> result = new ArrayList<>();
- for (org.apache.logging.log4j.Logger logger: childLoggers) {
- setLevel(logger, level);
- result.add(logger.getName());
+ Configurator.setAllLevels(internalNameSpace, level);
+ for (org.apache.logging.log4j.Logger logger : loggers) {
+ String name = getLoggerName(logger);
Review Comment:
please add comment to explain why we need to keep the "oldLevel"
##########
clients/src/test/java/org/apache/kafka/common/utils/LogCaptureAppender.java:
##########
@@ -138,7 +139,7 @@ public List<String> getMessages(String level) {
}
public List<String> getMessages() {
- final LinkedList<String> result = new LinkedList<>();
+ final List<String> result = new CopyOnWriteArrayList<>();
Review Comment:
Why to use `CopyOnWriteArrayList` as a local variable?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -103,18 +105,16 @@ public synchronized LoggerLevel level(String logger) {
* @return the levels of all known loggers; may be empty, but never null
*/
public synchronized Map<String, LoggerLevel> allLevels() {
- Map<String, LoggerLevel> result = new TreeMap<>();
-
- currentLoggers().stream()
- .filter(logger -> !logger.getLevel().equals(Level.OFF))
- .forEach(logger -> result.put(logger.getName(),
loggerLevel(logger)));
-
- org.apache.logging.log4j.Logger root = rootLogger();
- if (!root.getLevel().equals(Level.OFF)) {
- result.put(ROOT_LOGGER_NAME, loggerLevel(root));
- }
-
- return result;
+ return currentLoggers()
+ .values()
+ .stream()
+ .filter(logger -> !logger.getLevel().equals(Level.OFF))
+ .collect(Collectors.toMap(
+ this::getLoggerName,
+ this::loggerLevel,
+ (existing, replacing) -> replacing,
+ TreeMap::new)
Review Comment:
why to use `TreeMap`?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/LoggersTest.java:
##########
@@ -20,248 +20,168 @@
import org.apache.kafka.common.utils.Time;
import org.apache.kafka.connect.runtime.rest.entities.LoggerLevel;
-import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.core.LoggerContext;
-import org.apache.logging.log4j.core.config.Configuration;
-import org.apache.logging.log4j.core.config.Configurator;
-import org.apache.logging.log4j.core.config.LoggerConfig;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtendWith;
-import org.mockito.junit.jupiter.MockitoExtension;
-import org.mockito.junit.jupiter.MockitoSettings;
-import org.mockito.quality.Strictness;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.function.Function;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import static org.apache.logging.log4j.Level.DEBUG;
+import static org.apache.logging.log4j.Level.ERROR;
+import static org.apache.logging.log4j.Level.INFO;
+import static org.apache.logging.log4j.Level.WARN;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
-@ExtendWith(MockitoExtension.class)
-@MockitoSettings(strictness = Strictness.STRICT_STUBS)
public class LoggersTest {
-
private static final long INITIAL_TIME = 1696951712135L;
+ private final LoggerContext context = (LoggerContext)
LogManager.getContext(false);
+ private Loggers loggers;
private Time time;
@BeforeEach
public void setup() {
+ context.stop();
Review Comment:
please don't reset context - it results in the different loggers used by
"same" logger name, and that obstructs us from adding appenders correctly
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
