Dan Burkert has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 14: (11 comments) http://gerrit.cloudera.org:8080/#/c/7250/14/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 1140: ? new NonRecoverableException(statusTimedOut) >From a quick look, it seems like a null cause will be passed all the way up >the chain without being dereferenced, so I don't think the ternary is >necessary. Line 1382: extra space http://gerrit.cloudera.org:8080/#/c/7250/14/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 33: final class AuthnTokenReacquirer { Add visibility/privacy annotations. Line 38: private final Object affectedRpcInfoListLock = new Object(); Make the name of this match the list (there's an extra 'info' currently). Line 46: private ArrayList<KuduRpc<?>> affectedRpcList = Lists.newArrayList(); I think this would be better called 'queuedRpcs' or 'rpcsAwaitingToken'. Affected is a little bit generic. I prefer the first because it make the desired FIFO semantics explicit. Line 82: rpc.addTrace(new RpcTraceFrame.RpcTraceFrameBuilder( I was thinking that this trace frame should be added when the RPC is popped, so that the timestamp includes the time waiting in the queue, but I'm not entirely sure. I think we should get JD to weigh in. Line 93: private List<KuduRpc<?>> prepareRpcListToNotify() { This naming can be improved. The class is using a standard double-buffering technique, but the names and (lack of) comments don't make that clear. I'd suggest 'swapQueuedRpcs'. Line 96: Preconditions.checkState(!affectedRpcList.isEmpty()); Move this after the synchronized block (and therefore on the local instead of the field), and considering making this an 'assert', since the method is private and this would indicate a logic bug in the class. Line 109: @Override Put annotation after comments. Line 128: void handleAffectedRpcs() { naming: 'retryQueuedRpcs' would be more indicative of what is happening. PS14, Line 165: handleAffectedRpcs naming: 'failQueuedRpcs' -- 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: 14 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
