Alexey Serbin has posted comments on this change. Change subject: java-client: improve error messages when failing to connect to secure cluster ......................................................................
Patch Set 1: (5 comments) looks good, just a few nits http://gerrit.cloudera.org:8080/#/c/7824/1/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java: Line 370: nit: an extra line Line 383: nit: an extra-line PS1, Line 395: if (!(e instanceof SSLException) || !explicitlyDisconnected) { : LOG.error(message, e); : } While you are at it, maybe log the message with INFO priority about the Netty's confusion just to be able to track those cases for easier troubleshooting? http://gerrit.cloudera.org:8080/#/c/7824/1/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java File java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java: PS1, Line 298: private void handleNegotiateResponse(Channel chan, RpcHeader.NegotiatePB response) throws IOException { style: this line is more than 100 characters long. BTW, I see 'throws' statement moved from its own line and joined with the first line of the signature. What is the rationale behind? To me, having 'throws' in a separate line looks more readable. PS1, Line 380: if (chosenMech == null) { : String message; : : // TODO(KUDU-1948): when the Java client has an option to require security, detect the case : // where the server is configured without Kerberos and the client requires it. : if (serverMechs.size() == 1 && serverMechs.contains("GSSAPI")) { : // Give a better error diagnostic for common case of an unauthenticated client connecting : // to a secure server. : message = "Server requires Kerberos, but this client is not authenticated (kinit)"; : } else { : message = "Unable to negotiate a matching mechanism. Errors: [" + : Joiner.on(", ").withKeyValueSeparator(": ").join(errorsByMech) + "]"; : } : throw new NonRecoverableException(Status.ConfigurationError(message)); : } nit: maybe, get rid of extra-scope and replace it with: if (chosenMech != null) { return; } String message; ... throw new NonRecoverableException(Status.ConfigurationError(message)); -- To view, visit http://gerrit.cloudera.org:8080/7824 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I41c3229dcda284dce57cb6f6930efe3b50fa9698 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
