Dan Burkert 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) good point - test included. 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 Done Line 383: > nit: an extra-line Done 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 Net I think we are intentionally not logging this case because it was a frequent cause of red herring messages. 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. Done. 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: Done. I don't know if we have hard and fast rules about wrapping throws, but I personally dislike splitting throws from the exceptions. -- 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 <danburk...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
