You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by anmolnar <gi...@git.apache.org> on 2017/12/12 13:50:34 UTC
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. Make 'addr' variable available ...
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 <an...@cloudera.com>
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
----
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by anmolnar <gi...@git.apache.org>.
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.
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by paulmillar <gi...@git.apache.org>.
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.
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by anmolnar <gi...@git.apache.org>.
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.
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by phunt <gi...@git.apache.org>.
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?
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by anmolnar <gi...@git.apache.org>.
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```.
---
[GitHub] zookeeper issue #430: ZOOKEEPER-2893. very poor choice of logging if client ...
Posted by anmolnar <gi...@git.apache.org>.
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?
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/430
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by anmolnar <gi...@git.apache.org>.
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 ", ")
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by phunt <gi...@git.apache.org>.
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?
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by paulmillar <gi...@git.apache.org>.
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)
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by paulmillar <gi...@git.apache.org>.
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: {}"
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by paulmillar <gi...@git.apache.org>.
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]
---
[GitHub] zookeeper pull request #430: ZOOKEEPER-2893. very poor choice of logging if ...
Posted by anmolnar <gi...@git.apache.org>.
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.
---