chia7712 commented on code in PR #18185:
URL: https://github.com/apache/kafka/pull/18185#discussion_r1897309318
##########
tests/kafkatest/tests/connect/connect_distributed_test.py:
##########
@@ -563,7 +563,14 @@ def test_dynamic_logging(self, metadata_quorum):
# have been discarded
self._restart_worker(worker)
restarted_loggers = self.cc.get_all_loggers(worker)
- assert initial_loggers == restarted_loggers
+
+ for loggerName in restarted_loggers:
+ logger = self.cc.get_logger(worker, loggerName)
+ level = logger['level']
+ if loggerName ==
'org.apache.kafka.clients.consumer.ConsumerConfig':
Review Comment:
please add docs to say this logger is existent in the configuration
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -127,14 +126,28 @@ 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 = loggers.stream().collect(
+ Collectors.toMap(
+ this::getLoggerName,
+ org.apache.logging.log4j.Logger::getLevel,
+ (existing, replacing) -> replacing,
+ HashMap::new
Review Comment:
do we really need to avoid collision? `currentLoggers` returns the map
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -151,15 +164,13 @@ public synchronized List<String> setLevel(String
namespace, Level level) {
private synchronized List<org.apache.logging.log4j.Logger> loggers(String
namespace) {
Objects.requireNonNull(namespace, "Logging namespace may not be null");
- if (ROOT_LOGGER_NAME.equalsIgnoreCase(namespace)) {
- List<org.apache.logging.log4j.Logger> result = currentLoggers();
- result.add(rootLogger());
- return result;
+ if (isValidRootLoggerName(namespace)) {
+ return new ArrayList<>(currentLoggers().values());
Review Comment:
It seems `loggers` can return collection as well?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -151,15 +164,13 @@ public synchronized List<String> setLevel(String
namespace, Level level) {
private synchronized List<org.apache.logging.log4j.Logger> loggers(String
namespace) {
Objects.requireNonNull(namespace, "Logging namespace may not be null");
- if (ROOT_LOGGER_NAME.equalsIgnoreCase(namespace)) {
- List<org.apache.logging.log4j.Logger> result = currentLoggers();
- result.add(rootLogger());
- return result;
+ if (isValidRootLoggerName(namespace)) {
+ return new ArrayList<>(currentLoggers().values());
}
- List<org.apache.logging.log4j.Logger> result = new ArrayList<>();
- org.apache.logging.log4j.Logger ancestorLogger =
lookupLogger(namespace);
- List<org.apache.logging.log4j.Logger> currentLoggers =
currentLoggers();
+ var result = new ArrayList<org.apache.logging.log4j.Logger>();
+ var ancestorLogger = lookupLogger(namespace);
Review Comment:
maybe we should check all `currentLoggers` and then create the logger for
the input namespace if it is nonexistent?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Loggers.java:
##########
@@ -127,14 +126,28 @@ 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 = loggers.stream().collect(
+ Collectors.toMap(
+ this::getLoggerName,
+ org.apache.logging.log4j.Logger::getLevel,
+ (existing, replacing) -> replacing,
+ HashMap::new
+ )
+ );
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);
+ if (lookupLogger(name).getLevel() != nameToLevel.get(name)) {
Review Comment:
why to call `lookupLogger(name)`? we already have a logger, right?
--
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
