[GitHub] [zookeeper] tisonkun commented on a diff in pull request #1856: ZOOKEEPER-4515: ZK Cli quit command always logs error

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

tisonkun commented on code in PR #1856:
URL: https://github.com/apache/zookeeper/pull/1856#discussion_r846131093


##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java:
##########
@@ -1284,10 +1284,11 @@ public void run() {
                 } catch (Throwable e) {
                     if (closing) {
                         // closing so this is expected
-                        LOG.warn(
-                            "An exception was thrown while closing send thread for session 0x{}.",
-                            Long.toHexString(getSessionId()),
-                            e);
+                        if (LOG.isDebugEnabled()) {
+                            LOG.debug(
+                                "An exception was thrown while closing send thread for session 0x{}.",
+                                Long.toHexString(getSessionId()), e);
+                        }

Review Comment:
   IIUC `LOG.debug` already contains a call to `LOG.isDebugEnabled`.



##########
zookeeper-server/src/main/java/org/apache/zookeeper/util/ServiceUtils.java:
##########
@@ -39,7 +39,14 @@ private ServiceUtils() {
      */
     @SuppressFBWarnings("DM_EXIT")
     public static final Consumer<Integer> SYSTEM_EXIT = (code) -> {
-        LOG.error("Exiting JVM with code {}", code);

Review Comment:
   I'd prefer to change `LOG_ONLY` below also. Here is my diff:
   
   ```diff
   diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/util/ServiceUtils.java b/zookeeper-server/src/main/java/org/apache/zookeeper/util/ServiceUtils.java
   index 544e13d7f..09c45e93f 100644
   --- a/zookeeper-server/src/main/java/org/apache/zookeeper/util/ServiceUtils.java
   +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/util/ServiceUtils.java
   @@ -39,7 +39,11 @@ private ServiceUtils() {
         */
        @SuppressFBWarnings("DM_EXIT")
        public static final Consumer<Integer> SYSTEM_EXIT = (code) -> {
   -        LOG.error("Exiting JVM with code {}", code);
   +        if (code != 0) {
   +            LOG.error("Exiting JVM with code {}", code);
   +        } else {
   +            LOG.info("Exiting JVM with code {}", code);
   +        }
            System.exit(code);
        };
    
   @@ -47,8 +51,11 @@ private ServiceUtils() {
         * No-op strategy, useful for tests.
         */
        public static final Consumer<Integer> LOG_ONLY = (code) -> {
   -        LOG.error("Fatal error, JVM should exit with code {}. "
   -                + "Actually System.exit is disabled", code);
   +        if (code != 0) {
   +            LOG.error("Fatal error, JVM should exit with code {}. Actually System.exit is disabled", code);
   +        } else {
   +            LOG.info("JVM should exit with code {}. Actually System.exit is disabled", code);
   +        }
        };
    
        private static volatile Consumer<Integer> systemExitProcedure = SYSTEM_EXIT;
   @@ -56,8 +63,6 @@ private ServiceUtils() {
        /**
         * Override system callback. Useful for preventing the JVM to exit in tests
         * or in applications that are running an in-process ZooKeeper server.
   -     *
   -     * @param systemExitProcedure
         */
        public static void setSystemExitProcedure(Consumer<Integer> systemExitProcedure) {
            Objects.requireNonNull(systemExitProcedure);
   
   ```



-- 
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: notifications-unsubscribe@zookeeper.apache.org

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