You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/04/08 10:50:02 UTC
[GitHub] [zookeeper] arshadmohammad opened a new pull request, #1856: ZOOKEEPER-4515: ZK Cli quit command always logs error
arshadmohammad opened a new pull request, #1856:
URL: https://github.com/apache/zookeeper/pull/1856
1. For connection closing state scenario, changed the log level to debug
2. When JVM exiting with code 0, then logging info instead of error
--
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
[GitHub] [zookeeper] tisonkun commented on a diff in pull request #1856: ZOOKEEPER-4515: ZK Cli quit command always logs error
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #1856:
URL: https://github.com/apache/zookeeper/pull/1856#discussion_r846554629
##########
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:
Thanks for your explanation. Then we can keep as is.
--
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
[GitHub] [zookeeper] Govind85 commented on pull request #1856: ZOOKEEPER-4515: ZK Cli quit command always logs error
Posted by "Govind85 (via GitHub)" <gi...@apache.org>.
Govind85 commented on PR #1856:
URL: https://github.com/apache/zookeeper/pull/1856#issuecomment-1665718602
@arshadmohammad Could you please apply the same fix to 3.8.0 version?
It is showing same warning in 3.8.0 version and it is writing lot of logs.
--
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
[GitHub] [zookeeper] eolivelli commented on pull request #1856: ZOOKEEPER-4515: ZK Cli quit command always logs error
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #1856:
URL: https://github.com/apache/zookeeper/pull/1856#issuecomment-1093818549
Please chery pick to all active branches
--
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
[GitHub] [zookeeper] arshadmohammad commented on a diff in pull request #1856: ZOOKEEPER-4515: ZK Cli quit command always logs error
Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on code in PR #1856:
URL: https://github.com/apache/zookeeper/pull/1856#discussion_r846251275
##########
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:
It is not a core code flow. So for me it is ok to go either way.
I have put debug enable check to void unnecessary toHexString call.
shall we keep as it is or shall it change it?
--
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
[GitHub] [zookeeper] tisonkun commented on a diff in pull request #1856: ZOOKEEPER-4515: ZK Cli quit command always logs error
Posted by 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
[GitHub] [zookeeper] arshadmohammad commented on a diff in pull request #1856: ZOOKEEPER-4515: ZK Cli quit command always logs error
Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on code in PR #1856:
URL: https://github.com/apache/zookeeper/pull/1856#discussion_r846294591
##########
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:
good catch. included it.
--
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
[GitHub] [zookeeper] asfgit closed pull request #1856: ZOOKEEPER-4515: ZK Cli quit command always logs error
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1856: ZOOKEEPER-4515: ZK Cli quit command always logs error
URL: https://github.com/apache/zookeeper/pull/1856
--
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