Github user trkurc commented on the pull request:
https://github.com/apache/nifi/pull/97#issuecomment-150413539
The good news. It seems to work when I tested it.
One comment on style:
Having a tuple in a tuple for what I think is a triple for hdfsResources
was a little bit hard to read. Has it gotten big enough to be its own class
with getters for readability?
I really had to think about what this was doing, and whether the first
getValue could return null
```
hdfsResources.get().getValue().getValue())
```
this is very evident here where you created a method to read the
usergroupinfo and then replicate the work of the method the line below.
```
if (getUserGroupInformation() != null && isTicketOld()) {
renewKerberosTicket(hdfsResources.get().getValue().getValue());
}
...
protected UserGroupInformation getUserGroupInformation() {
return hdfsResources.get().getValue().getValue();
}
```
Other comments:
I was trying to reason about your error handling if
ugi.checkTGTAndReloginFromKeytab threw an IOException. If this was greenfield
code, I'd say that behavior I'd expect is that getFileSystem should throw an
IOException, but that would change the method signature and change all the
processors that use it (and be a breaking api change). Swallowing the
exception, then exponentially backing off on a retry almost seems more logical,
but I think it is worth discussing.
Is there a reason you divide the millisecond times by 1000 for comparisons
with seconds instead of multiplying the seconds by 1? You're losing precision
on the time stamp.
Did you think about trying to get the retry interval be a property instead
of being hardcoded?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
