Alexey Serbin has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 12: (19 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 Kerbero Done PS12, Line 297: Kerberos credentials > Here as well. Done Line 303: LOG.warn("connect to master: received a new authn token"); > WARN seems excessive for these messages; maybe debug? Done Line 303: LOG.warn("connect to master: received a new authn token"); > hm I'd vote INFO since this would only happen in a very long-running app, w Done Line 305: cb.call(Boolean.TRUE); > Can it just pass true/false here and below? it should auto-box. Done PS12, Line 307: connecto > connect Done Line 344: AuthnTokenReacquirer getTokenReacquirer() { > @VisibleForTesting? I updates the related code to be like 'handleRetriableRpc()' and other methods, so this accessor is not longer necessary. Thank you for pointing at this. 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 I keep the exception due to the 'technical' reasons: it's needed to retry the RPC using existing signature: it's needed to add the information about the retry into the trace. Line 108: > extra line Done PS12, Line 119: uccessu > succesful Done PS12, Line 119: attemp > attempt Done PS12, Line 122: {@link Boolean.TRUE} > These can just be {@code true} / {@code false} Done Line 125: public Void call(Boolean tokenAcquired) throws Exception { > tokenAcquired isn't used? yep, here it's not used. I'm not sure what to do in this case - just retry the operation? From the other case, the case when we got no token back and no exception detected manifests a problem on the server side. PS12, Line 133: Kerberos > primary Done 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 aggressiv That's a good idea. What if I just it just a TODO here for now? Line 155: this.attempts = 0; > this is now redundant with the field initializer Done 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 Yes, we could do that. But 1) I'm not sure it's safe to do it right here, in the handler 2) Failed negotiation is a special case which requires some special handling. Do you mind if I try to address this in a follow-up commit? 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 n It's a good observation. Done. http://gerrit.cloudera.org:8080/#/c/7250/12/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java: Line 36: @Nonnull > nit: again would prefer not to have these random changes in this commit. I' Sorry about that -- reverted. I agree it's better to put it separate unless it's really necessary in the changelist. -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
