chia7712 commented on code in PR #18185:
URL: https://github.com/apache/kafka/pull/18185#discussion_r1896504225
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -103,18 +105,15 @@ 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()
Review Comment:
we can allow `currentLoggers` return `HashMap<String,
org.apache.logging.log4j.Logger>` to ensure the de-duplicate.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -200,7 +198,7 @@ org.apache.logging.log4j.Logger rootLogger() {
private void setLevel(org.apache.logging.log4j.Logger logger, Level level)
{
String loggerName = logger.getName();
Review Comment:
ditto:
```java
var currentLevel = logger.getLevel();
if (level.equals(currentLevel)) {
log.debug("Skipping update for logger {} since its level is
already {}", logger.getName(), level);
return;
}
log.debug("Setting level of logger {} (excluding children) to {}",
logger.getName(), level);
Configurator.setLevel(logger.getName(), level);
lastModifiedTimes.put(logger.getName(), time.milliseconds());
```
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -220,4 +218,21 @@ private LoggerLevel
loggerLevel(org.apache.logging.log4j.Logger logger) {
Long lastModified = lastModifiedTimes.get(logger.getName());
return new LoggerLevel(Objects.toString(level), lastModified);
Review Comment:
Could we just return the level from logger? for example:
```java
return new LoggerLevel(Objects.toString(logger.getLevel()), lastModified);
```
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -179,16 +176,17 @@ private synchronized
List<org.apache.logging.log4j.Logger> loggers(String namesp
// visible for testing
org.apache.logging.log4j.Logger lookupLogger(String logger) {
- return LogManager.getLogger(logger);
+ return LogManager.getLogger(isValidRootLoggerName(logger) ?
LogManager.ROOT_LOGGER_NAME : logger);
}
List<org.apache.logging.log4j.Logger> currentLoggers() {
LoggerContext context = (LoggerContext) LogManager.getContext(false);
- Collection<LoggerConfig> loggerConfigs =
context.getConfiguration().getLoggers().values();
- return loggerConfigs.stream()
- .map(LoggerConfig::getName)
- .distinct()
- .map(LogManager::getLogger)
+ // Make sure root logger has been initialized
Review Comment:
Maybe we can collect loggers from configuration as well that the root logger
is must included.
```java
var results = new HashMap<String, org.apache.logging.log4j.Logger>();
context.getConfiguration().getLoggers().forEach((name, logger) ->
results.put(name, LogManager.getLogger(name)));
context.getLoggerRegistry().getLoggers().forEach(logger ->
results.put(logger.getName(), logger));
return results.values();
```
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -75,7 +77,7 @@ public synchronized LoggerLevel level(String logger) {
Objects.requireNonNull(logger, "Logger may not be null");
org.apache.logging.log4j.Logger foundLogger = null;
- if (ROOT_LOGGER_NAME.equalsIgnoreCase(logger)) {
+ if (isValidRootLoggerName(logger)) {
foundLogger = rootLogger();
} else {
List<org.apache.logging.log4j.Logger> currentLoggers =
currentLoggers();
Review Comment:
we can use `var` as much as possible to simplify code base
--
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]
