Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9050 )
Change subject: KUDU-2264. java: automatically attempt to re-acquire Kerberos credentials before expiration ...................................................................... Patch Set 3: (8 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@113 PS3, Line 113: associate them with a {@link javax.security.auth.Subject} : * instance, and associate them with the current thread of execution it will be a bit wordier, but I think you should avoid pronouns (them) in this sentence, it's not 100% clear what they refer to. http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@180 PS3, Line 180: This Specify that this is a function of UserGroupInformation http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@185 PS3, Line 185: The Kudu client emits : * DEB wrapping http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@221 PS3, Line 221: {@link InterfaceAudience.Private Are we giving up on ever changing an Unstable interface? Might be good to add a link to InterfaceStability. http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java File java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java: http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java@423 PS3, Line 423: if (s == null || : s.getPrivateCredentials(KerberosTicket.class).isEmpty()) { this can be one line now, which will read better 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@86 PS3, Line 86: private final Object subjectLock = new Object(); I don't know how you feel about this style, but since 'subject' is private, you could just use it directly as the lock. http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java File java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java: http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java@97 PS3, Line 97: options.put("debug", Boolean.toString(Boolean.getBoolean("kudu.jaas.debug"))); Is this trick worth adding to the debug section of your new doc? http://gerrit.cloudera.org:8080/#/c/9050/3/java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java@170 PS3, Line 170: return millisUntilEnd * 1000 < REFRESH_BEFORE_EXPIRATION_SECS; Shouldn't this be multiplying the expiration_secs by 1000 to make the units line up? -- 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: 3 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: Wed, 07 Mar 2018 19:21:59 +0000 Gerrit-HasComments: Yes
