Alexey Serbin has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 1: (21 comments) Thank you for the review. I'll address the major points in the next patch revision. Right now I wanted to quickly respond and bring the code up-to-date with the updated revision of the parent patch. http://gerrit.cloudera.org:8080/#/c/7250/1//COMMIT_MSG Commit Message: PS1, Line 9: current > 'the current' Done http://gerrit.cloudera.org:8080/#/c/7250/1/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: Line 272: private RpcProxy newRpcProxy(final ServerInfo serverInfo, boolean usePrimaryCredentials) { > Put this directly under the 1-arg version, and copy over the relevant javad Done http://gerrit.cloudera.org:8080/#/c/7250/1/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 38: public class AuthnTokenReacquirer { > Did you consider making this functionality part of SecurityContext? Is the Nope, I didn't think about making this is a part of SecurityContext. I'll give it a try, thanks. Line 48: private final ReentrantLock lock = new ReentrantLock(); > It looks like this could be simplified to just be an Object with synchroniz This sounds good, I will give it a try. Line 54: // TODO(aserbin): remove the client parameter > Looks like this TODO may be stale Done Line 65: // The token re-acquirer should be able to mark all the queued messages as failed if after some > Should this be a TODO? Right -- that was supposed to be a TODO. Done. Line 72: * @param connection connection to notify on the > sentence fragment Done Line 142: if (e instanceof RecoverableException && retries++ < 5) { > Could you move the increment of retries out of the if condition? It's diff Done Line 174: ConnectToCluster.run(masterTable, masterAddresses, null, > This won't reuse cached Master connections, right? Don't we do that on the This will reuse cached Master connection if the connection to the requested destination is present in the cache and established with primary credentials. http://gerrit.cloudera.org:8080/#/c/7250/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 102: private final ClientSocketChannelFactory channelFactory; > Doesn't look like this gets used outside the ctor? Right -- done. Line 272: if (m instanceof Negotiator.Failure) { > Could we get in a weird state here where this connection is actually a mast Yep, that's possible -- I replied on your comment on ConnectionCache.getConnection() method. Line 282: Preconditions.checkState(state == State.NEGOTIATING); > Might be worth adding a check that this connection is not using primary cre That's a good idea. Yes, if that happens, we should report an exception so it would be handled appropriately. Line 668: TO_BE_RENEGOTIATED, // The connection negotiation has failed and has to be be re-negotiated. > If I understand correctly, this patch is changing Connection so that it act Yep, ideally it would be nice to have just DISCONNECTED state and recycle disconnected connections. But then it's necessary to move the queued RPCs into a new connection. Probably, that can be done at the ConnectionCache level. I'll take a closer look at that. http://gerrit.cloudera.org:8080/#/c/7250/1/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 100: // 3. A connection with primary credentials is requested but currently registered one > Hmm, this comment would seem to contradict my earlier understanding about a Probably, your confusion comes from the naming for the 'DISCONNECTED' state. One small detail: when connection negotiation fails, the state changes to TO_BE_RENEGOTIATED, not DISCONNECTED. The DISCONNECTED state is a terminal state in the state diagram. And yes, a Connection object starts re-negotiation, after getting into TO_BE_RENEGOTIATED state. Such a connection will stay in the cache unless that's a connection to the master where we need to establish a new connection using primary credentials. In the latter case, the re-negotiation happens anyway, but the re-negotiated connection is replaced in the cache with the primary-credentials-connection-to-master which is used to re-acquire authn token. The queued RPCs from the re-negotiated old connection-to-the-same-master will be sent when the connection is re-negotiated using the new authn token. In that case there will be two connections from the client to the same master. I think it's not a big issue because there might be extra M connections at most, where M is number of master servers in the cluster (which is limited). A 'cleaner way' would be moving the queued RPCs from the old connection-to-the-same-master into the newly established connection once an authn token is acquired. However, there might be a race of closing the newly established connection and assigning queued RPCs from the old connection to the newly established one. One way of addressing that race would be allowing a connection in the DISCONNECTED state to re-connect. I didn't like that idea. However, there is another way of addressing that -- handling the queued RPCs using re-acquirer itself. I'll address that. http://gerrit.cloudera.org:8080/#/c/7250/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: Line 236: if (header.hasIsError() && header.getIsError()) { > I think this can be simplified to if (header.getIsError()) since the defaul Done Line 240: LOG.debug("connection negotiation error: " + error.getMessage()); > Use slf4j string interpolation: Done Line 716: static class Result { > Perhaps this should be renamed 'Success' to better indicate it's use. Done http://gerrit.cloudera.org:8080/#/c/7250/1/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 37: // servers even for not-yet-expired tokens. > .. tablet servers, even for ... Done Line 49: c.disconnect(); > Kind of ironic that it's called 'getImmutableConnectionsList' :) Yep, that's an inconsistent naming -- agreed. I tried to preserve the original naming. As I understand, the idea was to reflect the fact the _list_ is immutable, i.e. removing/adding connections from/to the result container does not affect the actual set of client's connections. I'll rename it. Line 62: ListTabletServersResponse response = syncClient.listTabletServers(); > Just to ratchet up the intensity a bit, maybe run this test case in a loop That's a good idea. Done. http://gerrit.cloudera.org:8080/#/c/7250/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquireOpen.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquireOpen.java: Line 53: FakeDNS.getInstance().install(); > What about this setup is different from the other test class? Maybe commen Here we want to have just 1 master to simplify the logic of the test. I will add a comment. -- 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: 1 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
