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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
