Alexey Serbin has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 10: (20 comments) 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. Yes, this comment sounds upside-down and messed up. Right, if there is no token but only Kerberos credentials, then the Kerberos credentials will be used. I'll update this accordingly. 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 Done PS10, Line 260: Preconditions.checkNotNull(serverInfo), > The Guava guide recommends putting Preconditions checks on their own line s Indeed -- using checkNotNull() would not make much sense here since it's the same NPE, but coming just from this particular place instead of the point of actual de-reference. I'll remove it from here and other similar places. PS10, Line 1337: Status reasonForRetry > this variable isn't used until down below the if statement -- move it down Done PS10, Line 1337: Preconditions.checkNotNull > what's the purpose of this? given you are about to dereference it, it would Agreed -- I rather mechanically placed Preconditions everywhere in the new code. I will update this and other places. Line 1749: return tableId.equals(MASTER_TABLE_NAME_PLACEHOLDER); > this is a purposeful object instance equality check (see the comment above I looked at it and this seems safe since this method is private. Agreed that sneaking unrelated changes into bigger changelists is not a good idea. 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? Done 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 It's a great idea. PS10, Line 111: Preconditions.checkNotNull(rpc), Preconditions.checkNotNull(ex > prefer to see these as the first thing in a function (so it clearly states Yep, but then the linter complains. I think it's better to move the non-null precondition check into the AffectedRpcInfo constructor, as discussed somewhere else. Line 135: final class NewAuthnTokenCB implements Callback<Void, ConnectToClusterResponse> { > woah, I didn't know you could define a class inside a function. can/should I tried to make it static but the compiler wasn't happy about that: Error:(136, 18) java: modifier static not allowed here After some research, I found more info where this restriction is stated explicitly: http://docs.oracle.com/javase/tutorial/java/javaOO/localclasses.html https://docstore.mik.ua/orelly/java-ent/jnut/ch03_11.htm PS10, Line 136: NewAuthnTokenCB() { : } > remove Done PS10, Line 149: success > this gets called even if it didn't get a new token, right? Right -- I updated the comment and the code in the call() method as well. PS10, Line 173: 5 > where'd this constant come from? maybe extract it to a NUM_RETRIES or somet Done 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 "Cre Done PS10, Line 578: Preconditions.checkNotNull(inflightMessages) > found this easier to read non-inline I removed it as per the discussion: use the checkNotNull only when storing the references for future access. 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? Done PS10, Line 132: credentialsPolicy != Connection.CredentialsPolicy.PRIMARY_CREDENTIALS > I think this would read better as == ANY_CREDENTIALS rather than != PRIMARY Done PS10, Line 133: Connection.CredentialsPolicy.PRIMARY_CREDENTIALS > maybe just use 'credentialsPolicy' here? yep, and that's better for the readability. Line 145: Preconditions.checkState(connections.size() <= 2); > more of an assert IMO Done 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 This is about returning a Throwable instance without throwing it out (HashMap.put() returns Throwable). It's the suppression for IntelliJ Idea code inspector: Analyze > Inspect Code...). -- 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: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
