Kai Zheng commented on HADOOP-13590:

Looking at the codes closely:
1. Maybe you also want to show {{now}} and {{renewalFailures}} values in the 
warning log?
+                LOG.warn("Exception encountered while running the renewal"
+                    + " command for {}.", getUserName(), ie);
+                final long now = Time.now();
+                nextRefresh =
+                    getNextRetryTime(tgt, now, metrics.renewalFailures);
+                metrics.renewalFailures++;

2. Could be renamed and more specific: getNextTgtRenewalTime. And static. If 
you pass {{tgtEndTime}} instead of the {{tgt}}, it would make 
{{testGetNextRetryTime}} test much more simplified.
  long getNextRetryTime(final KerberosTicket tgt, final long currentTime,
      final long failureCount) {
    LOG.debug("Tgt endtime is {}, failure count is {}.",
        tgt.getEndTime().getTime(), failureCount);
    final long lastRetryTime =
        tgt.getEndTime().getTime() - kerberosMinSecondsBeforeRelogin;
    return Math.min(lastRetryTime,
        currentTime + kerberosMinSecondsBeforeRelogin * (1 << failureCount));

3. A suggestion by the way, not introduced by this and not sure if it's good to 
do it here. Could we return earlier at the beginning so we can avoid at least 2 
level of indents and make the whole block more readable?
  /**Spawn a thread to do periodic renewals of kerberos credentials*/
  private void spawnAutoRenewalThreadForUserCreds() {
    if (isSecurityEnabled()) {
      //spawn thread only if we have kerb credentials
      if (user.getAuthenticationMethod() == AuthenticationMethod.KERBEROS &&
          !isKeytab) {
                             very deep nested ...

4. Just a question. Any other exception than {{IOException}} could be thrown 

5. In the new test class {{TestUGIWithMiniKdc}}: I'm not sure if we need 
{{testUGI}} to doAs the call 
+      loginContext.login();
+      final Subject loginSubject = loginContext.getSubject();
+      final UserGroupInformation testUGI =
+          UserGroupInformation.createUserForTesting("testing", new String[0]);
+      testUGI.doAs(new PrivilegedExceptionAction<Void>() {
+        @Override
+        public Void run() throws IOException {
+          UserGroupInformation.loginUserFromSubject(loginSubject);
+          return null;
+        }
+      });

> Retry until TGT expires even if the UGI renewal thread encountered exception
> ----------------------------------------------------------------------------
>                 Key: HADOOP-13590
>                 URL: https://issues.apache.org/jira/browse/HADOOP-13590
>             Project: Hadoop Common
>          Issue Type: Improvement
>          Components: security
>    Affects Versions: 2.8.0, 2.7.3, 2.6.4
>            Reporter: Xiao Chen
>            Assignee: Xiao Chen
>         Attachments: HADOOP-13590.01.patch, HADOOP-13590.02.patch, 
> HADOOP-13590.03.patch, HADOOP-13590.04.patch, HADOOP-13590.05.patch
> The UGI has a background thread to renew the tgt. On exception, it 
> [terminates 
> itself|https://github.com/apache/hadoop/blob/bee9f57f5ca9f037ade932c6fd01b0dad47a1296/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java#L1013-L1014]
> If something temporarily goes wrong that results in an IOE, even if it 
> recovered no renewal will be done and client will eventually fail to 
> authenticate. We should retry with our best effort, until tgt expires, in the 
> hope that the error recovers before that.

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

Reply via email to