Dan Burkert has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 1: (21 comments) http://gerrit.cloudera.org:8080/#/c/7250/1//COMMIT_MSG Commit Message: PS1, Line 9: current 'the current' 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 javadoc. 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 there an advantage to keeping it separate? Line 48: private final ReentrantLock lock = new ReentrantLock(); It looks like this could be simplified to just be an Object with synchronized blocks, since the reentrant features aren't used. Line 54: // TODO(aserbin): remove the client parameter Looks like this TODO may be stale 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? Line 72: * @param connection connection to notify on the sentence fragment Line 142: if (e instanceof RecoverableException && retries++ < 5) { Could you move the increment of retries out of the if condition? It's difficult to determine that it's doing the right thing when it's on the right hand side of a short-circuiting &&. Line 174: ConnectToCluster.run(masterTable, masterAddresses, null, This won't reuse cached Master connections, right? Don't we do that on the C++ side? 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? Line 272: if (m instanceof Negotiator.Failure) { Could we get in a weird state here where this connection is actually a master connection, but not using primary credentials, and this error handling actually spins up a completely new master Connection? In general, I'm a little uneasy with the possibility that connections are being created outside of the client's connection cache now. Line 282: Preconditions.checkState(state == State.NEGOTIATING); Might be worth adding a check that this connection is not using primary credentials 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 actually re-connects after a negotiation failure? Why not keep it simple and keep the 1 TCP Socket to 1 Connection relationship? 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 Connection now retrying negotiation internally, so I'm a little confused. FWIW this comment is how I would expect things to work. 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 default is false. Line 240: LOG.debug("connection negotiation error: " + error.getMessage()); Use slf4j string interpolation: LOG.debug("connection negotiation error: {}" error.getMessage()) Line 716: static class Result { Perhaps this should be renamed 'Success' to better indicate it's use. 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 ... (add comma) Line 49: c.disconnect(); Kind of ironic that it's called 'getImmutableConnectionsList' :) Line 62: ListTabletServersResponse response = syncClient.listTabletServers(); Just to ratchet up the intensity a bit, maybe run this test case in a loop and/or with many threads? 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 comment that specifically. -- 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 <[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
