Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9050 )
Change subject: KUDU-2264. java: automatically attempt to re-acquire Kerberos credentials ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/9050/3/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: http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@140 PS3, Line 140: * the ticket cache to obtain a new ticket with a later expiration time. So, if > Do we need to mention that Kudu will refuse the re-login attempt if the pri I don't think so because it is more like us trying to prevent a weird edge case we expect to never happen. I think providing too much detail in this context may just confuse the reader. http://gerrit.cloudera.org:8080/#/c/9050/3/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: http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@95 PS3, Line 95: static > 'static' is redundant for inner enums. Done http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@107 PS3, Line 107: . > Nit: add ')' Done http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@130 PS3, Line 130: * @param subject JAAS Subject that the client's credentials are stored in > Nit: remove the extra @param. Done http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@171 PS3, Line 171: public void refreshSubject() { > Nit: Maybe add a brief comment in front of the function since it is public. Done http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@191 PS3, Line 191: > Extra line. actually I like this extra line because the above comment refers to the rest of the function. If I remove this line it makes it look like it's referring to the if statement below. http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java: http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@55 PS3, Line 55: static > Redundant. Done http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@118 PS3, Line 118: miniCluster.killMasters(); > I do not quite follow why do we have to kill the masters. Does it mean as l yep, because the connection has already been established, keeping it alive means you never need to re-authenticate. I'll clarify that point. http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@273 PS3, Line 273: public void testRenewAndReacquireKeberosCredentials() throws Exception { > Nit: addd a comment here? Done http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@355 PS3, Line 355: Closeable c > Not used? For a "try-with-resources" block I think it's required to name the resource variable even if it's never used: https://stackoverflow.com/questions/16588843/why-does-try-with-resource-require-a-local-variable http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@386 PS3, Line 386: Closeable c > Same here. See above -- To view, visit http://gerrit.cloudera.org:8080/9050 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I514253e0a7f067dbc8ffe4eaf5a7a2c32900b539 Gerrit-Change-Number: 9050 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Anonymous Coward #380 Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 08 Mar 2018 02:44:54 +0000 Gerrit-HasComments: Yes
