Dan Burkert has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/7250/8/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 676: TERMINATED, +1 on this name. Definitely makes it more clear that this is the final state, and it can't be changed after this. http://gerrit.cloudera.org:8080/#/c/7250/8/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 114: foundTerminated = true; This may be simpler if you use a named iterator to cycle through, and call remove() if the connection is terminated. That way you don't need a flag / have to do another loop through. http://gerrit.cloudera.org:8080/#/c/7250/8/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java: Line 83: * the securityContext expires I think this comment may be stale: 'if the current one expires.' Line 343: private final class AuthnTokenReacquirer { Hmm despite what I said I'm not sure this is better than when it was split out. I was more imagining the functionality would be entirely subsumed into SecurityContext (just a single class), but maybe that's too much for one class. The test-only ctor of the parent class is definitely a smell. http://gerrit.cloudera.org:8080/#/c/7250/8/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 93: LOG.error("unexpected exception in test scenario", e); Will this cause the test to fail? -- 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: 8 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
