Alexey Serbin has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 8: (24 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/ Done 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 Is it better to use assert instead of Preconditions here? Could you elaborate why is that, please? I'm not sure assert is a better choice here since doesn't enforce those checks unless the JVM is run with the '-ea' flag. PS8, Line 279: : // Setting state to FAILED_NEGOTIATION. > nit: redundant with the below code Done 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 exce Yes, I thought about this. The problem was with keeping the enqueued messages/RPCs somewhere -- I didn't want to do double buffering and have more complex code (the queue in the client level should be per-server). Also, for the time while a new authn token is acquired, it's better to 'plug' particular destination so the client will not try to open and negotiation a new connection in vain, thrashing the connection cache. I started with the design where it's not necessary to serialize RPCs again, and it was possible to re-connect currently existing connection -- upon re-connection the enqueued wire-format messages would be sent as necessary. But then there was a request to not re-open a connection, so it ended up like this -- some sort of compromise. If we value clean separation of responsibilities more, then sure it's a way to go. I think it might be the best option -- I'll update this. PS8, Line 289: securityContext.reacquireAuthnToken(this); > seems like it would make more sense to me to call back up into the client h If the re-acquisition logic is moved up, then sure -- it should be done from the higher level. 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 Not exactly -- I was thinking making it in the previous changelist (separating Connection). I just thought it's easier to call Channels.close() regardless of whether the channel is open and then do clean-up from here. Line 482: return Channels.close(channel); > same here, close vs disconnect can be subtle, not sure why it's in this pat Yes, it might. But as I understand if we don't want to keep the channel around, it's better to close right here. Not sure postponing closing the channel buys us anything here. Also, all tests are passing now. If we are concerned with that, I'm return it back then. Line 649: // The object has just been created. > now that you're moving the comments into the appropriate position, mind cha Done 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, renaming into AWAITING_NEW_AUTHN_TOKEN would be better if using current design. However, if switching to the design where the token re-acquisition is started by the upper level, it might make sense to either remove this state or leave this naming. Line 676: TERMINATED, > +1 on this name. Definitely makes it more clear that this is the final sta Yep, I changed it from DISCONNECTED because of that and because FAILED_NEGOTIATION implies the connection is de-facto disconnected already. 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 explaini Done PS8, Line 89: boolean usePrimaryCredentials > nit: javadoc this Done Line 114: foundTerminated = true; > This may be simpler if you use a named iterator to cycle through, and call ok, this sounds good, thanks. Line 123: if (foundTerminated) { > don't you want to remove the terminated connection even if you found a non- It's more like an optimization -- remove terminated connections when adding new instead of them. Yes, it looks a little bit strange, but number of masters is limited. It will be naturally updated after using to named iterators and doing everything on the first pass. PS8, Line 140: checkState > isn't this an assert() not a Precondition? ie its failure would indicate a Yes, it's more a post-condition. Precondition.checkState() is used here to check for internal state. It might be assert, but asserts are disabled in run-time unless -ea is specified for jvm. What's wrong with using Preconditions library for consistency checks like this? 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? Done PS8, Line 720: // The RPC error received from the server. > nit: use /** ... */ Done 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 Done Line 83: * the securityContext expires > I think this comment may be stale: 'if the current one expires.' Done PS8, Line 129: this.tokenReacquirer = new AuthnTokenReacquirer(masterAddresses, masterTable, : connectToClusterTimeoutMs); > can we avoid the coupling of KuduTable and masterAddresses etc by just taki Yup, once AuthnTokenReacquirer is separated back, this will be automatically resolved. Line 343: private final class AuthnTokenReacquirer { > hrm, I didn't see the original rev, but I thought it would be less coupling ok, that makes sense -- I will update this. The original rev is PS1. Line 343: private final class AuthnTokenReacquirer { > Hmm despite what I said I'm not sure this is better than when it was split ok, np. I will separate it back. 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? Yes. In the original version callback functions were named differently, so it was easier to duplicate this tiny piece of code than instead of trying to re-use. http://gerrit.cloudera.org:8080/#/c/7250/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquire.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquire.java: Line 93: LOG.error("unexpected exception in test scenario", e); > Will this cause the test to fail? Nope, but I'll update it to do so. Thanks! -- 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
