belugabehr commented on a change in pull request #2633:
URL: https://github.com/apache/hadoop/pull/2633#discussion_r561251354



##########
File path: 
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/DelegationTokenRenewer.java
##########
@@ -223,7 +223,7 @@ static synchronized void reset() {
     if (action.token != null) {
       queue.add(action);
     } else {
-      fs.LOG.error("does not have a token for renewal");
+      FileSystem.LOG.error("does not have a token for renewal");

Review comment:
       Bad practice here.  Accessing a `static` value in a non-static way.

##########
File path: 
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/DelegationTokenRenewer.java
##########
@@ -260,8 +259,7 @@ public void run() {
       } catch (InterruptedException ie) {
         return;
       } catch (Exception ie) {
-        action.weakFs.get().LOG.warn("Failed to renew token, action=" + action,
-            ie);
+        FileSystem.LOG.warn("Failed to renew token, action=" + action, ie);

Review comment:
       Bad practice here. Accessing a static value in a non-static way.

##########
File path: 
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
##########
@@ -3391,15 +3391,7 @@ private static void loadFileSystems() {
               LOGGER.info("Full exception loading: {}", fs, e);
             }
           } catch (ServiceConfigurationError ee) {
-            LOG.warn("Cannot load filesystem: " + ee);
-            Throwable cause = ee.getCause();
-            // print all the nested exception messages
-            while (cause != null) {
-              LOG.warn(cause.toString());
-              cause = cause.getCause();
-            }
-            // and at debug: the full stack
-            LOG.debug("Stack Trace", ee);
+            LOGGER.warn("Cannot load filesystem", ee);

Review comment:
       I'm not sure what the capability of Commons Logger is, but in SLF4J, the 
entire list of 'caused by' is printed.

##########
File path: 
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
##########
@@ -3391,15 +3391,7 @@ private static void loadFileSystems() {
               LOGGER.info("Full exception loading: {}", fs, e);
             }
           } catch (ServiceConfigurationError ee) {
-            LOG.warn("Cannot load filesystem: " + ee);
-            Throwable cause = ee.getCause();
-            // print all the nested exception messages
-            while (cause != null) {
-              LOG.warn(cause.toString());
-              cause = cause.getCause();
-            }
-            // and at debug: the full stack
-            LOG.debug("Stack Trace", ee);
+            LOGGER.warn("Cannot load filesystem", ee);

Review comment:
       I'm not sure what the capability of Commons Logger is, but in SLF4J, the 
entire list of 'caused by' is printed so there is no need to do this manually 
when switching loggers, but this may also just be an oversight, I'm not sure.

##########
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/TestFcHdfsSetUMask.java
##########
@@ -100,16 +97,6 @@ public static void ClusterShutdownAtEnd() throws Exception {
       cluster.shutdown();
     }
   }
-  
-  {
-    try {
-      GenericTestUtils.setLogLevel(FileSystem.LOG, Level.DEBUG);
-    }
-    catch(Exception e) {
-      System.out.println("Cannot change log level\n"
-          + StringUtils.stringifyException(e));
-    }
-  }

Review comment:
       Not good practice to modify this manually.  Should be using log4j 
properties files to set logging levels.




----------------------------------------------------------------
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.

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

Reply via email to