[GitHub] [hadoop] steveloughran commented on a diff in pull request #5456: HADOOP-18653. LogLevel servlet to determine log impl before using setLevel

2023-03-10 Thread via GitHub


steveloughran commented on code in PR #5456:
URL: https://github.com/apache/hadoop/pull/5456#discussion_r1132513808


##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericsUtil.java:
##
@@ -103,14 +113,15 @@ public static boolean isLog4jLogger(Class clazz) {
* @return true if the logger uses Log4J implementation.
*/
   public static boolean isLog4jLogger(String logger) {
-if (logger == null) {
+if (logger == null || !IS_LOG4J_LOGGER.get()) {
   return false;
 }
 Logger log = LoggerFactory.getLogger(logger);
 try {

Review Comment:
   this try/catch is a duplicate of isLog4jLogger(class). 
   why not do `isLog4jLogger(LoggerFactory.getLogger(logger))`



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org



[GitHub] [hadoop] steveloughran commented on a diff in pull request #5456: HADOOP-18653. LogLevel servlet to determine log impl before using setLevel

2023-03-08 Thread via GitHub


steveloughran commented on code in PR #5456:
URL: https://github.com/apache/hadoop/pull/5456#discussion_r1129648239


##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/log/LogLevel.java:
##
@@ -338,14 +341,18 @@ public void doGet(HttpServletRequest request, 
HttpServletResponse response
 out.println(MARKER
 + "Submitted Class Name: " + logName + "");
 
-Logger log = Logger.getLogger(logName);
+org.slf4j.Logger log = LoggerFactory.getLogger(logName);
 out.println(MARKER
 + "Log Class: " + log.getClass().getName() +"");
 if (level != null) {
   out.println(MARKER + "Submitted Level: " + level + "");
 }
 
-process(log, level, out);
+if (GenericsUtil.isLog4jLogger(logName)) {
+  process(Logger.getLogger(logName), level, out);
+} else {
+  out.println("Sorry, " + log.getClass() + " not supported.");

Review Comment:
   text to explain "log4j loggers only"



##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericsUtil.java:
##
@@ -89,10 +89,30 @@ public static boolean isLog4jLogger(Class clazz) {
 }
 Logger log = LoggerFactory.getLogger(clazz);
 try {
-  Class log4jClass = Class.forName("org.slf4j.impl.Log4jLoggerAdapter");
+  Class log4jClass = Class.forName("org.slf4j.impl.Log4jLoggerAdapter");

Review Comment:
   make this a constant string and use everywhere it is needed



##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericsUtil.java:
##
@@ -89,10 +89,30 @@ public static boolean isLog4jLogger(Class clazz) {
 }
 Logger log = LoggerFactory.getLogger(clazz);
 try {
-  Class log4jClass = Class.forName("org.slf4j.impl.Log4jLoggerAdapter");
+  Class log4jClass = Class.forName("org.slf4j.impl.Log4jLoggerAdapter");
   return log4jClass.isInstance(log);
 } catch (ClassNotFoundException e) {
   return false;
 }
   }
+
+  /**
+   * Determine whether the log of the given logger is of Log4J implementation.
+   *
+   * @param logger the logger name, usually class name as string.
+   * @return true if the logger uses Log4J implementation.
+   */
+  public static boolean isLog4jLogger(String logger) {
+if (logger == null) {
+  return false;
+}
+Logger log = LoggerFactory.getLogger(logger);
+try {
+  Class log4jClass = Class.forName("org.slf4j.impl.Log4jLoggerAdapter");

Review Comment:
   if the class isn't found, then that fact can be remembered in an atomic 
boolean so future loads/checks skipped. 



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org