Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upZOOKEEPER-2893. very poor choice of logging if client fails to connect to server #430
Conversation
anmolnar
changed the title from
ZOOKEEPER-2893. Make 'addr' variable available for error handling code to give a chance to fallback if the socket hasn't been initialized yet
to
ZOOKEEPER-2893. very poor choice of logging if client fails to connect to server
Dec 12, 2017
| LOG.warn( | ||
| "Session 0x" | ||
| + Long.toHexString(getSessionId()) | ||
| + " for server " | ||
| + clientCnxnSocket.getRemoteSocketAddress() | ||
| + (remoteAddr == null ? addr : remoteAddr) |
This comment has been minimized.
This comment has been minimized.
paulmillar
Dec 13, 2017
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -1041,6 +1041,8 @@ private void sendPing() { | ||
| private InetSocketAddress rwServerAddress = null; | ||
| private InetSocketAddress addr = null; |
This comment has been minimized.
This comment has been minimized.
paulmillar
Dec 13, 2017
[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)
This comment has been minimized.
This comment has been minimized.
anmolnar
Dec 13, 2017
Contributor
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.
| @@ -1236,7 +1237,7 @@ public void run() { | ||
| "Session 0x" | ||
| + Long.toHexString(getSessionId()) | ||
| + " for server " | ||
| + clientCnxnSocket.getRemoteSocketAddress() | ||
| + serverAddress | ||
| + ", unexpected error" | ||
| + RETRY_CONN_MSG, e); |
This comment has been minimized.
This comment has been minimized.
phunt
Dec 14, 2017
Contributor
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?
This comment has been minimized.
This comment has been minimized.
anmolnar
Dec 14, 2017
Contributor
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.
| @@ -1041,6 +1041,8 @@ private void sendPing() { | ||
| private InetSocketAddress rwServerAddress = null; | ||
| private InetSocketAddress serverAddress = null; |
This comment has been minimized.
This comment has been minimized.
phunt
Dec 14, 2017
Contributor
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?
This comment has been minimized.
This comment has been minimized.
anmolnar
force-pushed the
anmolnar:ZOOKEEPER-2893
branch
from
84ed736
to
47a8cf4
Dec 14, 2017
| + clientCnxnSocket.getRemoteSocketAddress() | ||
| + ", unexpected error" | ||
| + RETRY_CONN_MSG, e); | ||
| LOG.warn("Session 0x{} for server {}, unexpected error{}", |
This comment has been minimized.
This comment has been minimized.
paulmillar
Dec 15, 2017
[non-blocking]
Although this isn't a regression, you probably want a ": " here (and a space); e.g.,
"Session 0x{} for server {}, unexpected error: {}"
This comment has been minimized.
This comment has been minimized.
anmolnar
Dec 15, 2017
Contributor
No, because the RETRY_CONN_MSG should go right after it. (starts with ", ")
This comment has been minimized.
This comment has been minimized.
|
@paulmillar Please approve it, if you're happy with the change. |
asfgit
closed this
in
e129e7a
Dec 19, 2017
pushed a commit
that referenced
this pull request
Dec 19, 2017
pushed a commit
that referenced
this pull request
Dec 19, 2017
pushed a commit
to lvfangmin/zookeeper
that referenced
this pull request
Jun 17, 2018
anmolnar commentedDec 12, 2017
'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.