Todd Lipcon has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 8: (19 comments) http://gerrit.cloudera.org:8080/#/c/7250/8/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: PS8, Line 229: helper here and below s/helper/proxy/ http://gerrit.cloudera.org:8080/#/c/7250/8/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: PS8, Line 276: Preconditions.checkState(state == State.NEGOTIATING); : Preconditions.checkState these are assertions not preconditions PS8, Line 279: : // Setting state to FAILED_NEGOTIATION. nit: redundant with the below code Line 281: // channelDisconnected() or channelClosed() until notified on the authn token re-acquisition results. high level design question: did you consider actually just throwing an exception on all of the pending RPCs (ie call their errback) but with a specific exception subclass or status code, and then change the error handling/retry logic in AsyncKuduClient to trigger the reacquireAuthnToken() stuff? That seems like it would avoid adding this rather kudu-specific stuff into Connection which seems to regress from the nice improvement you recently made to try to make connections more "KRPC" level constructs. PS8, Line 289: securityContext.reacquireAuthnToken(this); seems like it would make more sense to me to call back up into the client here I think? Line 377: // Explicitly close the channel (regardless whether it's disconnected) and call cleanup(). is this change related? I remember there's a lot of subtlety in this stuff last time I tried to touch it, and makes me nervous to "sneak in" to an unrelated patch Line 482: return Channels.close(channel); same here, close vs disconnect can be subtle, not sure why it's in this patch Line 649: // The object has just been created. now that you're moving the comments into the appropriate position, mind changing them to /** javadoc style */ ? I think that way they'll show up in IDEs when hovered (not sure if IDEs would show it as is) PS8, Line 661: negotiation error and there are : // enqueued messages to handle. Upon failed negotiation event, the Connection object notifies : // its securityContext to start acquiring a new authn token this only happens in the case of the specific invalid authn token failure, right? can you clarify, and maybe clarify the state name like AWAITING_NEW_AUTHN_TOKEN or something? http://gerrit.cloudera.org:8080/#/c/7250/8/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 68: private final HashMultimap<String, Connection> uuid2connection = HashMultimap.create(); now that this is a multimap, I think it warrants more of a comment explaining what the multiple connections per UUID are PS8, Line 89: boolean usePrimaryCredentials nit: javadoc this and potentially introduce an enum for CredentialsPolicy or something so we get nicer parameter names at call sites? Line 123: if (foundTerminated) { don't you want to remove the terminated connection even if you found a non-terminated one above? also agreed with Dan on using an iterator PS8, Line 140: checkState isn't this an assert() not a Precondition? ie its failure would indicate a bug in this class's implementation, not a misuse by a caller http://gerrit.cloudera.org:8080/#/c/7250/8/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: PS8, Line 241: LOG.debug("connection negotiation error: {}", error.getMessage()); can this include the peer address or uuid etc? PS8, Line 720: // The RPC error received from the server. nit: use /** ... */ http://gerrit.cloudera.org:8080/#/c/7250/8/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java: Line 47: import com.stumbleupon.async.Callback; nit: probably goes in the previous section of non-kudu includes PS8, Line 129: this.tokenReacquirer = new AuthnTokenReacquirer(masterAddresses, masterTable, : connectToClusterTimeoutMs); can we avoid the coupling of KuduTable and masterAddresses etc by just taking the AuthnTokenReacquirer as a ctor parameter and constructing it when we create the context? That would also potentially allow easy mocking in the negotiation test, and avoid having the near-duplicate constructor above. Line 343: private final class AuthnTokenReacquirer { > Hmm despite what I said I'm not sure this is better than when it was split hrm, I didn't see the original rev, but I thought it would be less coupling if this were separate as well. I like keeping SecurityContext as a "dumb" object that just stores credentials and related utility code. PS8, Line 460: /** Notify affected connections on the failure of re-acquire authn token. */ : void notifyAffectedConnections() { : HashSet<Connection> connections; : synchronized (affectedConnectionsLock) { : Preconditions.checkState(!affectedConnections.isEmpty()); : connections = affectedConnections; : affectedConnections = Sets.newHashSet(); : } : : for (final Connection c : connections) { : c.notifyAuthnTokenAcquired(false); : } : } this code can be shared with above right? -- 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: 8 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
