[GitHub] [helix] qqu0127 commented on a diff in pull request #2208: Unnecessary log removal

[GitBox <gi...@apache.org>]

qqu0127 commented on code in PR #2208:
URL: https://github.com/apache/helix/pull/2208#discussion_r967278621


##########
helix-core/src/main/java/org/apache/helix/tools/commandtools/JmxDumper.java:
##########
@@ -211,7 +211,6 @@ void init() throws Exception {
       System.out.println(_outputFileName);
       _outputFile = new PrintWriter(fos);
     } catch (Exception e) {
-      _logger.error("fail to get all existing mbeans in " + _domain, e);

Review Comment:
   We don't even need the try-catch if no action is made on exception.



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -2066,14 +2066,11 @@ public Object call() throws Exception {
         success = true;
       } catch (ZkNoNodeException e) {
         success = false;
-        if (LOG.isDebugEnabled()) {
-          LOG.debug("zkclient {}, Failed to delete path {}, znode does not exist!", _uid, path);
-        }
+        LOG.debug("zkclient {}, Failed to delete path {}, znode does not exist!", _uid, path);

Review Comment:
   nit: we can append the exception to the log message.



##########
helix-core/src/main/java/org/apache/helix/tools/commandtools/TaskAdmin.java:
##########
@@ -124,7 +124,6 @@ public static void main(String[] args) throws Exception {
         throw new IllegalArgumentException("Unknown command " + args[0]);
       }
     } catch (IllegalArgumentException e) {
-      LOG.error("Unknown driver command " + args[0]);

Review Comment:
   same



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -2066,14 +2066,11 @@ public Object call() throws Exception {
         success = true;
       } catch (ZkNoNodeException e) {
         success = false;
-        if (LOG.isDebugEnabled()) {
-          LOG.debug("zkclient {}, Failed to delete path {}, znode does not exist!", _uid, path);
-        }
+        LOG.debug("zkclient {}, Failed to delete path {}, znode does not exist!", _uid, path);
       }
       record(path, null, startT, ZkClientMonitor.AccessType.WRITE);
     } catch (Exception e) {
       recordFailure(path, ZkClientMonitor.AccessType.WRITE);
-      LOG.warn("zkclient {}, Failed to delete path {}! ", _uid, path, e);

Review Comment:
   Rather a general comment, let's be cautious on the removal. The thrown exception may not contain local context like `_uid` and `path`, so it may make debugging harder. 
   `ZkClient` is a very fundamental class, my personal take is to keep it, no strong preference though.



##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -277,9 +277,6 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node, boo
           .error(String.format("Current cluster %s has issue with health checks!", clusterId), e);
       throw new HelixHealthException(e);
     } catch (Exception e) {
-      _logger.error(String.format(
-              "Failed to get parallel stoppable instances for cluster %s with a HelixException!",
-              clusterId), e);

Review Comment:
   same



-- 
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: reviews-unsubscribe@helix.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org