Todd Lipcon has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 10: (20 comments) looks pretty good, mostly cosmetic comments. I like this design much better http://gerrit.cloudera.org:8080/#/c/7250/10/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: PS10, Line 233: The result proxy object fits for : * sending RPCs which can be sent over a connection negotiated with secondary credentials : * (e.g. authn token) this makes it sound like a restriction. Really don't you mean that "this doesn't restrict the type of credentials that may be used to connect to the server"? ie if you have Kerberos credentials and no token, it would use those, right? PS10, Line 235: as for time : * writing this code, tablet servers do not serve any RPCs which change their behavior depending : * on the type of authentication credentials. not sure we need this level of detail about tserver specifics PS10, Line 260: Preconditions.checkNotNull(serverInfo), The Guava guide recommends putting Preconditions checks on their own line so it's a little clearer to trace back when they fail: https://github.com/google/guava/wiki/PreconditionsExplained (they suggest using the inline checkNotNull() result for field initializers) PS10, Line 1337: Preconditions.checkNotNull what's the purpose of this? given you are about to dereference it, it would throw an NPE either way (Preconditions.checkNotNull just throws NPE). I think checkNotNull is more useful in cases where you plan to store a reference or otherwise propagate it rather than just immediately dereference it PS10, Line 1337: Status reasonForRetry this variable isn't used until down below the if statement -- move it down for better clarity? Line 1749: return tableId.equals(MASTER_TABLE_NAME_PLACEHOLDER); this is a purposeful object instance equality check (see the comment above it). Agreed it is weird. (btw see my earlier comment about unrelated cleanup changes) http://gerrit.cloudera.org:8080/#/c/7250/10/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 40: public final class AffectedRpcInfo { can this be static? PS10, Line 50: private final AsyncKuduClient client; : private final SecurityContext securityContext; : private final List<HostAndPort> masterAddresses; : private final KuduTable masterTable; : private final long defaultAdminOperationTimeoutMs; since the client has all of the other 4 fields inside of it, could we avoid having to take all of these in by making a package-private method like 'client.reconnectToCluster(cb, errb)'? PS10, Line 111: Preconditions.checkNotNull(rpc), Preconditions.checkNotNull(ex prefer to see these as the first thing in a function (so it clearly states all the preconditions right at the top) Line 135: final class NewAuthnTokenCB implements Callback<Void, ConnectToClusterResponse> { woah, I didn't know you could define a class inside a function. can/should this be static? (same below) PS10, Line 136: NewAuthnTokenCB() { : } remove PS10, Line 149: success this gets called even if it didn't get a new token, right? PS10, Line 173: 5 where'd this constant come from? maybe extract it to a NUM_RETRIES or something? http://gerrit.cloudera.org:8080/#/c/7250/10/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 117: * Whether to negotiate the connection using primary credentials; i.e. whether to ignore the this javadoc sounds like a boolean field. I think just changing to say "Credentials policy to use when authenticating" or something would be fine, since you already have a very nice javadoc on the CredentialsPolicy type itself PS10, Line 578: Preconditions.checkNotNull(inflightMessages) found this easier to read non-inline http://gerrit.cloudera.org:8080/#/c/7250/10/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: PS10, Line 123: Iterator shouldn't this be Iterator<Connection> so you don't need the cast below? PS10, Line 132: credentialsPolicy != Connection.CredentialsPolicy.PRIMARY_CREDENTIALS I think this would read better as == ANY_CREDENTIALS rather than != PRIMARY PS10, Line 133: Connection.CredentialsPolicy.PRIMARY_CREDENTIALS maybe just use 'credentialsPolicy' here? Line 145: Preconditions.checkState(connections.size() <= 2); more of an assert IMO http://gerrit.cloudera.org:8080/#/c/7250/10/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 99: //noinspection ThrowableResultOfMethodCallIgnored hm, which lint tool is this suppressing? just curious -- 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: 10 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: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
