[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Hudson (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16297257#comment-16297257
 ] 

Hudson commented on ZOOKEEPER-2893:
---

SUCCESS: Integrated in Jenkins build ZooKeeper-trunk #3665 (See 
[https://builds.apache.org/job/ZooKeeper-trunk/3665/])
ZOOKEEPER-2893: very poor choice of logging if client fails to connect (phunt: 
rev e129e7a0b64d6555460d240be2d79e53aaa1bef9)
* (edit) src/java/main/org/apache/zookeeper/ClientCnxn.java


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16294827#comment-16294827
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user anmolnar commented on the issue:

https://github.com/apache/zookeeper/pull/430
  
@paulmillar Please approve it, if you're happy with the change.
@phunt Do you think it can be committed?


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16292967#comment-16292967
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r157266599
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1231,14 +1231,14 @@ public void run() {
 LOG.info(e.getMessage() + RETRY_CONN_MSG);
 } else if (e instanceof RWServerFoundException) {
 LOG.info(e.getMessage());
+} else if (e instanceof SocketException) {
+LOG.info("Socket error occurred: {}: {}", 
serverAddress, e.getMessage());
 } else {
-LOG.warn(
-"Session 0x"
-+ 
Long.toHexString(getSessionId())
-+ " for server "
-+ 
clientCnxnSocket.getRemoteSocketAddress()
-+ ", unexpected error"
-+ RETRY_CONN_MSG, e);
+LOG.warn("Session 0x{} for server {}, 
unexpected error{}",
--- End diff --

No, because the RETRY_CONN_MSG should go right after it. (starts with ", ")


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16292545#comment-16292545
 ] 

Hadoop QA commented on ZOOKEEPER-2893:
--

+1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+0 tests included.  The patch appears to be a documentation patch that 
doesn't require tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

+1 core tests.  The patch passed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1375//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1375//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1375//console

This message is automatically generated.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16292541#comment-16292541
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user paulmillar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r157201054
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1231,14 +1231,14 @@ public void run() {
 LOG.info(e.getMessage() + RETRY_CONN_MSG);
 } else if (e instanceof RWServerFoundException) {
 LOG.info(e.getMessage());
+} else if (e instanceof SocketException) {
+LOG.info("Socket error occurred: {}: {}", 
serverAddress, e.getMessage());
 } else {
-LOG.warn(
-"Session 0x"
-+ 
Long.toHexString(getSessionId())
-+ " for server "
-+ 
clientCnxnSocket.getRemoteSocketAddress()
-+ ", unexpected error"
-+ RETRY_CONN_MSG, e);
+LOG.warn("Session 0x{} for server {}, 
unexpected error{}",
--- End diff --

[non-blocking]

Although this isn't a regression, you probably want a ": " here (and a 
space); e.g.,

"Session 0x{} for server {}, unexpected error: {}"


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Andor Molnar (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16292522#comment-16292522
 ] 

Andor Molnar commented on ZOOKEEPER-2893:
-

[~paulmillar]

Done.
Let's move this review to github, if you still have comments.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Paul Millar (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16291327#comment-16291327
 ] 

Paul Millar commented on ZOOKEEPER-2893:


Yes, that sounds fine.

One final thing: log4j has a built-in template support.  You can write the log 
message like:

LOG.info("Socket error occurred: {}: {}", serverAddress, e.getMessage());

In addition to being shorter, this has the advantage of avoiding the string 
concatenation if the message isn't going to be logged.  This is particularly 
useful for DEBUG-level and TRACE-level logging.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Andor Molnar (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16291144#comment-16291144
 ] 

Andor Molnar commented on ZOOKEEPER-2893:
-

[~paulmillar]

I've created a separate 'if' for SocketException, but not for IOException. Do 
you think we can live with that for now?

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16290951#comment-16290951
 ] 

Hadoop QA commented on ZOOKEEPER-2893:
--

+1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+0 tests included.  The patch appears to be a documentation patch that 
doesn't require tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

+1 core tests.  The patch passed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1369//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1369//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1369//console

This message is automatically generated.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16290905#comment-16290905
 ] 

Hadoop QA commented on ZOOKEEPER-2893:
--

-1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+0 tests included.  The patch appears to be a documentation patch that 
doesn't require tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

-1 core tests.  The patch failed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1368//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1368//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1368//console

This message is automatically generated.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16290884#comment-16290884
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r156952287
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1236,7 +1237,7 @@ public void run() {
 "Session 0x"
 + 
Long.toHexString(getSessionId())
 + " for server "
-+ 
clientCnxnSocket.getRemoteSocketAddress()
++ serverAddress
 + ", unexpected error"
 + RETRY_CONN_MSG, e);
--- End diff --

We're talking about the same thing in the Jira. It's arguable which 
exception should be logged at INFO level without the stack trace, I ended up 
separating SocketExceptions altogether and leaving the rest for the original 
handler.


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16290883#comment-16290883
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r156951799
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1041,6 +1041,8 @@ private void sendPing() {
 
 private InetSocketAddress rwServerAddress = null;
 
+private InetSocketAddress serverAddress = null;
--- End diff --

Good idea. I've made the change.


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Paul Millar (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16290571#comment-16290571
 ] 

Paul Millar commented on ZOOKEEPER-2893:


Catching NoRouteToHostException would be good, but there are other exceptions 
that can happen legitimately and that (I believe) shouldn't result in logging a 
stack-trace: ConnectException and PortUnreachableException for example.

Perhaps it would be better to catch SocketException and log it without a 
stack-trace?

The same argument could also applies to IOException; I'm not sure how useful it 
is to log a stack-trace on an UnknownHostException.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16290147#comment-16290147
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user phunt commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r156825456
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1041,6 +1041,8 @@ private void sendPing() {
 
 private InetSocketAddress rwServerAddress = null;
 
+private InetSocketAddress serverAddress = null;
--- End diff --

This seems kinda bogus to me - why push this up to a field. Can't we 
determine the server address in "run" method, as a local variable, and call 
startConnect with that as an argument? That seems like an improvement to 
startConnect call at the same time. What do you think @anmolnar , does that 
make sense or am I missing something?


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16290148#comment-16290148
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user phunt commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r156825263
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1236,7 +1237,7 @@ public void run() {
 "Session 0x"
 + 
Long.toHexString(getSessionId())
 + " for server "
-+ 
clientCnxnSocket.getRemoteSocketAddress()
++ serverAddress
 + ", unexpected error"
 + RETRY_CONN_MSG, e);
--- End diff --

I think it would be good to address the other issue Paul mentioned - no 
need to dump a stack if we know this is NoRouteToHostException - why wouldn't 
we add another elseif to check for this type?


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Andor Molnar (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16289437#comment-16289437
 ] 

Andor Molnar commented on ZOOKEEPER-2893:
-

[~paulmillar]

I thought about it, but that would be a non backward compatible change. This 
part is the catch-all exception handler so all non-expected errors will end up 
here which makes sense for dumping the whole stack trace.

I'd rather implement specific error handlers for NoRouteToHost and other 
special errors if you want to make it fancy.

Additionally, I'm not familiar with log4j/slf4j settings, but that could be a 
setting of the appender whether stack traces needs to be logged or not.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Paul Millar (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16289222#comment-16289222
 ] 

Paul Millar commented on ZOOKEEPER-2893:


The other thing that would be good to fix is that the message is logged with a 
stack-trace.

Would it be possible to change the log message so it doesn't include a 
stack-trace; for example, {{LOG.warn("Session 0x" +  ... + 
String.valueOf(e))}}, instead of {{LOG.warn("Session 0x" + ..., e)}} ?


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16289186#comment-16289186
 ] 

Hadoop QA commented on ZOOKEEPER-2893:
--

+1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+0 tests included.  The patch appears to be a documentation patch that 
doesn't require tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

+1 core tests.  The patch passed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1359//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1359//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1359//console

This message is automatically generated.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16288941#comment-16288941
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r156599702
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1041,6 +1041,8 @@ private void sendPing() {
 
 private InetSocketAddress rwServerAddress = null;
 
+private InetSocketAddress addr = null;
--- End diff --

Remote side of the connection. Basically the ip address that the client is 
trying to connect to. ```rwServerAddress``` is the address of non-R/O server 
found if there's any.

I'll change this to ```serverAddress```.


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16288924#comment-16288924
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r156597536
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1232,11 +1233,12 @@ public void run() {
 } else if (e instanceof RWServerFoundException) {
 LOG.info(e.getMessage());
 } else {
+SocketAddress remoteAddr = 
clientCnxnSocket.getRemoteSocketAddress();
 LOG.warn(
 "Session 0x"
 + 
Long.toHexString(getSessionId())
 + " for server "
-+ 
clientCnxnSocket.getRemoteSocketAddress()
++ (remoteAddr == null ? addr : 
remoteAddr)
--- End diff --

Makes sense. I'll just use 'addr' here.


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16288909#comment-16288909
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user paulmillar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r156594949
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1232,11 +1233,12 @@ public void run() {
 } else if (e instanceof RWServerFoundException) {
 LOG.info(e.getMessage());
 } else {
+SocketAddress remoteAddr = 
clientCnxnSocket.getRemoteSocketAddress();
 LOG.warn(
 "Session 0x"
 + 
Long.toHexString(getSessionId())
 + " for server "
-+ 
clientCnxnSocket.getRemoteSocketAddress()
++ (remoteAddr == null ? addr : 
remoteAddr)
--- End diff --

Of course this comment is [non-blocking]


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16288908#comment-16288908
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user paulmillar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r156594864
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1041,6 +1041,8 @@ private void sendPing() {
 
 private InetSocketAddress rwServerAddress = null;
 
+private InetSocketAddress addr = null;
--- End diff --

[non-blocking]

The name could be an ambiguous: is this the address of the local (client) 
side of the connection, or the remote (server) side of the connection?  
Especially as there is `rwServerAddress` field member defined immediately above.

Suggest `serverAddress` (or similar)


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16288906#comment-16288906
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

Github user paulmillar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/430#discussion_r156594153
  
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1232,11 +1233,12 @@ public void run() {
 } else if (e instanceof RWServerFoundException) {
 LOG.info(e.getMessage());
 } else {
+SocketAddress remoteAddr = 
clientCnxnSocket.getRemoteSocketAddress();
 LOG.warn(
 "Session 0x"
 + 
Long.toHexString(getSessionId())
 + " for server "
-+ 
clientCnxnSocket.getRemoteSocketAddress()
++ (remoteAddr == null ? addr : 
remoteAddr)
--- End diff --

I believe that, if `clientCnxnSocket.getRemoteSocketAddress()` is non-null, 
the value is always the same as `addr`.

If so, then this could be simplified to just `addr` without the ternary 
operation.


> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16287649#comment-16287649
 ] 

Hadoop QA commented on ZOOKEEPER-2893:
--

+1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+0 tests included.  The patch appears to be a documentation patch that 
doesn't require tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

+1 core tests.  The patch passed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1340//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1340//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/1340//console

This message is automatically generated.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Andor Molnar (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16287625#comment-16287625
 ] 

Andor Molnar commented on ZOOKEEPER-2893:
-

[~paulmillar] please check it 

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16287622#comment-16287622
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2893:
---

GitHub user anmolnar opened a pull request:

https://github.com/apache/zookeeper/pull/430

ZOOKEEPER-2893. Make 'addr' variable available for error handling code to 
give a chance to fallback if the socket hasn't been initialized yet

'addr' variable is used to identify which server to connect to.
I've made this available for error handling code in order to let it 
fallback to this address if the remote socket hasn't been initialised yet. This 
will give us better error messages if the client is unable to connect to server 
for some reason.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/anmolnar/zookeeper ZOOKEEPER-2893

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/430.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #430


commit fbe4ccde8516150cfd69d2a2260266fd1c0bf10d
Author: Andor Molnar 
Date:   2017-12-12T13:46:34Z

ZOOKEEPER-2893. Make 'addr' variable available for error handling code to 
give a chance to fallback if the socket hasn't been initialized yet




> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Andor Molnar (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16287581#comment-16287581
 ] 

Andor Molnar commented on ZOOKEEPER-2893:
-

[~paulmillar] I'll take a look and submit a pull request soon.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Andor Molnar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[Paul Millar (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16171675#comment-16171675
 ] 

Paul Millar commented on ZOOKEEPER-2893:


... just to add my 2c-worth

Sure, ClientCnxnSocket#getRemoteSocketAddress returns null; however the problem 
(item 3.) is not with the return value of 
ClientCnxnSocket#getRemoteSocketAddress.

Rather, the problem (item 3. in the list) is that the _error message_ is built 
using something that can be `null`.

The remote address is actually well-defined.  ClientCnxn certainly should know 
to which endpoint it is trying to connect.

To fix this problem, the error message should include the remote endpoint using 
something that cannot be null.

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>Assignee: Tamas Penzes
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2893) very poor choice of logging if client fails to connect to server

[maoling (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2893?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16167189#comment-16167189
 ] 

maoling commented on ZOOKEEPER-2893:


it is an unexpected exception:NoRouteToHostException, so 
ClientCnxnSocket#getRemoteSocketAddress returns null.for an unexpected 
exception,we can't know the reason why it happened and log its stack-trace for 
locating the problem

> very poor choice of logging if client fails to connect to server
> 
>
> Key: ZOOKEEPER-2893
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2893
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: java client
>Affects Versions: 3.4.6
>Reporter: Paul Millar
>
> We are using ZooKeeper in our project and have received reports that, when 
> suffering a networking problem, log files become flooded with messages like:
> {quote}
> 07 Sep 2017 08:22:00 (System) [] Session 0x45d3151be3600a9 for server null, 
> unexpected error, closing socket connection and attempting reconnect
> java.net.NoRouteToHostException: No route to host
> at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method) 
> ~[na:1.8.0_131]
> at 
> sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717) 
> ~[na:1.8.0_131]
> at 
> org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
>  ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> at 
> org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1081) 
> ~[zookeeper-3.4.6.jar:3.4.6-1569965]
> {quote}
> Looking at the code that logs this message ({{ClientCnxn}}), there seems to 
> be quite a few problems here:
> # the code logs a stack-trace, even though there is no bug here.  In our 
> project, we treat all logged stack-traces as bugs,
> # if the networking issue is not fixed promptly, the log files is flooded 
> with these message,
> # The message is built using {{ClientCnxnSocket#getRemoteSocketAddress}}, yet 
> in this case, this does not provide the expected information (yielding 
> {{null}}),
> # The log message fails to include a description of what actually went wrong.
> (Additionally, the code uses string concatenation rather than templating when 
> building the message; however, this is an optimisation issue)
> My suggestion is that this log entry is updated so that it doesn't log a 
> stack-trace, but does include some indication why the connection failed.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)