Todd Lipcon has posted comments on this change. ( )

Change subject: KUDU-2264. java: automatically attempt to re-acquire Kerberos 

Patch Set 4:

File java/kudu-client/src/main/java/org/apache/kudu/client/
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.
File java/kudu-client/src/main/java/org/apache/kudu/client/
PS3, Line 95: static
> 'static' is redundant for inner enums.
PS3, Line 107: .
> Nit: add ')'
PS3, Line 130:    * @param subject JAAS Subject that the client's credentials 
are stored in
> Nit: remove the extra @param.
PS3, Line 171:   public void refreshSubject() {
> Nit: Maybe add a brief comment in front of the function since it is public.
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.
File java/kudu-client/src/test/java/org/apache/kudu/client/
PS3, Line 55: static
> Redundant.
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.
PS3, Line 273:   public void testRenewAndReacquireKeberosCredentials() throws 
Exception {
> Nit: addd a comment here?
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:
PS3, Line 386: Closeable c
> Same here.
See above

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I514253e0a7f067dbc8ffe4eaf5a7a2c32900b539
Gerrit-Change-Number: 9050
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Anonymous Coward #380
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Hao Hao <>
Gerrit-Reviewer: Jean-Daniel Cryans <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-Comment-Date: Thu, 08 Mar 2018 02:44:54 +0000
Gerrit-HasComments: Yes

Reply via email to