Dan Burkert has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 12: (16 comments) http://gerrit.cloudera.org:8080/#/c/7250/12/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: PS12, Line 234: Kerberos credentials It may be more accurate to say 'SASL' credentials, since it will be Kerberos if available, or PLAIN otherwise. PS12, Line 297: Kerberos credentials Here as well. Line 303: LOG.warn("connect to master: received a new authn token"); WARN seems excessive for these messages; maybe debug? Line 305: cb.call(Boolean.TRUE); Can it just pass true/false here and below? it should auto-box. PS12, Line 307: connecto connect Line 344: AuthnTokenReacquirer getTokenReacquirer() { @VisibleForTesting? http://gerrit.cloudera.org:8080/#/c/7250/12/java/kudu-client/src/main/java/org/apache/kudu/client/AuthnTokenReacquirer.java File java/kudu-client/src/main/java/org/apache/kudu/client/AuthnTokenReacquirer.java: Line 37: public static final class AffectedRpcInfo { Shouldn't the fact that an RPC got delayed waiting for a token go into it's trace? Why the need to also keep around the exception separately? I think the RPC should never fail with the same exception; the only thing that can happen after being popped from this queue are a timeout, or a retry (which will have it's own exception if it fails). Line 108: extra line PS12, Line 119: attemp attempt PS12, Line 119: uccessu succesful PS12, Line 122: {@link Boolean.TRUE} These can just be {@code true} / {@code false} Line 125: public Void call(Boolean tokenAcquired) throws Exception { tokenAcquired isn't used? PS12, Line 133: Kerberos primary Line 145: * Errback to retry authn token re-acquisition and notify the handle the affected RPCs if the Perhaps we should retry indefinitely with increasing backoff, but aggressively timeout RPCs in the queue after each failure? http://gerrit.cloudera.org:8080/#/c/7250/12/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 669: NEGOTIATION_FAILED, The control flow around NEGOTIATION_FAILED is non-local, and hard to follow. If I understand correctly, we receive a message indicating that negotiation failed inside messageReceived, then we shove the failure condition into a field, and then wait for cleanup() to be called, wherin we do something special because that field is set. Could this be simplified by doing the special negotiation failed cleanup directly in messageReceived, then going directly to TERMINATED state? That might simplify the non-local control flow, and remove one of these states. http://gerrit.cloudera.org:8080/#/c/7250/12/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java: Line 69: private final ReentrantLock lock = new ReentrantLock(); It appears this no longer needs the re-entrant features, so it could be a normal Object with synchronized blocks (or even synchronize on uuid2connection directly). -- To view, visit http://gerrit.cloudera.org:8080/7250 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0be620629c9a8345ecd5e5679c80ee76ca4eaa57 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> 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-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
