Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-13590

Retry until TGT expires even if the UGI renewal thread encountered exception

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.8.0, 2.7.3, 2.6.4
    • Fix Version/s: 2.8.0, 3.0.0-alpha2
    • Component/s: security
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      The UGI has a background thread to renew the tgt. On exception, it terminates itself

      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.

      1. HADOOP-13590.01.patch
        7 kB
        Xiao Chen
      2. HADOOP-13590.02.patch
        8 kB
        Xiao Chen
      3. HADOOP-13590.03.patch
        11 kB
        Xiao Chen
      4. HADOOP-13590.04.patch
        11 kB
        Xiao Chen
      5. HADOOP-13590.05.patch
        14 kB
        Xiao Chen
      6. HADOOP-13590.06.patch
        13 kB
        Xiao Chen
      7. HADOOP-13590.07.patch
        13 kB
        Xiao Chen
      8. HADOOP-13590.08.patch
        18 kB
        Xiao Chen
      9. HADOOP-13590.09.patch
        18 kB
        Xiao Chen
      10. HADOOP-13590.10.patch
        18 kB
        Xiao Chen
      11. HADOOP-13590.branch-2.01.patch
        12 kB
        Xiao Chen

        Issue Links

          Activity

          Hide
          xiaochen Xiao Chen added a comment -

          Attaching a patch for the idea.

          Current behavior is to just print a exception message on the first failure. Feels to me we should retry here, since the UGI had a successful login. This would help in scenarios where the renew failure happen to be intermittent.

          Patch 1 just to have the retry to be simply every kerberosMinSecondsBeforeRelogin interval, until it succeeds or the tgt expires. We could add a more sophisticated retry logic (e.g. max # of retries with exponential backoff) if people like this direction.

          Also added the exception stack trace to the log message, instead of just the exception msg alone.

          Show
          xiaochen Xiao Chen added a comment - Attaching a patch for the idea. Current behavior is to just print a exception message on the first failure. Feels to me we should retry here, since the UGI had a successful login. This would help in scenarios where the renew failure happen to be intermittent. Patch 1 just to have the retry to be simply every kerberosMinSecondsBeforeRelogin interval, until it succeeds or the tgt expires. We could add a more sophisticated retry logic (e.g. max # of retries with exponential backoff) if people like this direction. Also added the exception stack trace to the log message, instead of just the exception msg alone.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 7m 3s trunk passed
          +1 compile 6m 52s trunk passed
          +1 checkstyle 0m 25s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 18s trunk passed
          +1 javadoc 0m 44s trunk passed
          +1 mvninstall 0m 36s the patch passed
          +1 compile 6m 48s the patch passed
          +1 javac 6m 48s the patch passed
          -0 checkstyle 0m 25s hadoop-common-project/hadoop-common: The patch generated 2 new + 148 unchanged - 0 fixed = 150 total (was 148)
          +1 mvnsite 0m 54s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 29s the patch passed
          +1 javadoc 0m 45s the patch passed
          +1 unit 7m 51s hadoop-common in the patch passed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          38m 33s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828190/HADOOP-13590.01.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 8b9435787cae 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / e3f7f58
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10496/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10496/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10496/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 3s trunk passed +1 compile 6m 52s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 18s trunk passed +1 javadoc 0m 44s trunk passed +1 mvninstall 0m 36s the patch passed +1 compile 6m 48s the patch passed +1 javac 6m 48s the patch passed -0 checkstyle 0m 25s hadoop-common-project/hadoop-common: The patch generated 2 new + 148 unchanged - 0 fixed = 150 total (was 148) +1 mvnsite 0m 54s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 29s the patch passed +1 javadoc 0m 45s the patch passed +1 unit 7m 51s hadoop-common in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 38m 33s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828190/HADOOP-13590.01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 8b9435787cae 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e3f7f58 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10496/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10496/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10496/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Improved the test to run faster. And fixed precommit warnings.
          The javac for getter method should be ignored IMO, to keep consistency with current code. Reviews appreciated!

          Show
          xiaochen Xiao Chen added a comment - Improved the test to run faster. And fixed precommit warnings. The javac for getter method should be ignored IMO, to keep consistency with current code. Reviews appreciated!
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          -1 patch 0m 5s HADOOP-13590 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



          Subsystem Report/Notes
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828711/HADOOP-13590.02.patch
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10522/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 5s HADOOP-13590 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828711/HADOOP-13590.02.patch Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10522/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          oops, rebased.

          Show
          xiaochen Xiao Chen added a comment - oops, rebased.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 6m 43s trunk passed
          +1 compile 6m 49s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 17s trunk passed
          +1 javadoc 0m 46s trunk passed
          +1 mvninstall 0m 37s the patch passed
          +1 compile 6m 45s the patch passed
          +1 javac 6m 45s the patch passed
          -0 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 1 new + 148 unchanged - 0 fixed = 149 total (was 148)
          +1 mvnsite 0m 54s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 26s the patch passed
          +1 javadoc 0m 45s the patch passed
          +1 unit 8m 14s hadoop-common in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          38m 27s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828714/HADOOP-13590.02.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 83087c88e4d2 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / fcbac00
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10523/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10523/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10523/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 43s trunk passed +1 compile 6m 49s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 17s trunk passed +1 javadoc 0m 46s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 6m 45s the patch passed +1 javac 6m 45s the patch passed -0 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 1 new + 148 unchanged - 0 fixed = 149 total (was 148) +1 mvnsite 0m 54s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 26s the patch passed +1 javadoc 0m 45s the patch passed +1 unit 8m 14s hadoop-common in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 38m 27s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828714/HADOOP-13590.02.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 83087c88e4d2 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / fcbac00 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10523/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10523/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10523/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Patch 3 added exponential backoff to make this nicer. Also added username to the log, to be consistent with current code.
          Could any of the watchers please take a look? Thanks a lot.

          Show
          xiaochen Xiao Chen added a comment - Patch 3 added exponential backoff to make this nicer. Also added username to the log, to be consistent with current code. Could any of the watchers please take a look? Thanks a lot.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 7m 14s trunk passed
          +1 compile 7m 2s trunk passed
          +1 checkstyle 0m 25s trunk passed
          +1 mvnsite 0m 58s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 22s trunk passed
          +1 javadoc 0m 46s trunk passed
          +1 mvninstall 0m 39s the patch passed
          +1 compile 6m 58s the patch passed
          +1 javac 6m 58s the patch passed
          -0 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 1 new + 148 unchanged - 0 fixed = 149 total (was 148)
          +1 mvnsite 0m 56s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 34s the patch passed
          +1 javadoc 0m 44s the patch passed
          +1 unit 7m 54s hadoop-common in the patch passed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          39m 27s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828940/HADOOP-13590.03.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux dbe77c2dcc6f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 58bae35
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10530/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10530/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10530/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 14s trunk passed +1 compile 7m 2s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 58s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 22s trunk passed +1 javadoc 0m 46s trunk passed +1 mvninstall 0m 39s the patch passed +1 compile 6m 58s the patch passed +1 javac 6m 58s the patch passed -0 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 1 new + 148 unchanged - 0 fixed = 149 total (was 148) +1 mvnsite 0m 56s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 34s the patch passed +1 javadoc 0m 44s the patch passed +1 unit 7m 54s hadoop-common in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 39m 27s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828940/HADOOP-13590.03.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux dbe77c2dcc6f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 58bae35 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10530/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10530/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10530/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          LGTM, though other Kerberos people need to look at the code too...this is so sensitive we almost need multiple votes on it.

          Can you add an accessor to stop that checkstyle warning?

          Show
          stevel@apache.org Steve Loughran added a comment - LGTM, though other Kerberos people need to look at the code too...this is so sensitive we almost need multiple votes on it. Can you add an accessor to stop that checkstyle warning?
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks for the review, Steve. Attached patch 4 to pass checkstyle.
          Any suggestion on whose vote we should ask for?

          I'm pinging Aaron T. Myers Robert Kanter Chris Nauroth Larry McCay: dear Kerberos people, could you please review? Thanks in advance!

          Show
          xiaochen Xiao Chen added a comment - Thanks for the review, Steve. Attached patch 4 to pass checkstyle. Any suggestion on whose vote we should ask for? I'm pinging Aaron T. Myers Robert Kanter Chris Nauroth Larry McCay : dear Kerberos people, could you please review? Thanks in advance!
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Owen O'Malley may have something to say, even if he will deny writing the original code

          Show
          stevel@apache.org Steve Loughran added a comment - Owen O'Malley may have something to say, even if he will deny writing the original code
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 10s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 6m 58s trunk passed
          +1 compile 8m 15s trunk passed
          +1 checkstyle 0m 25s trunk passed
          +1 mvnsite 1m 0s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 31s trunk passed
          +1 javadoc 0m 50s trunk passed
          +1 mvninstall 0m 46s the patch passed
          +1 compile 8m 7s the patch passed
          +1 javac 8m 7s the patch passed
          +1 checkstyle 0m 27s the patch passed
          +1 mvnsite 1m 0s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 50s the patch passed
          +1 javadoc 0m 53s the patch passed
          +1 unit 12m 22s hadoop-common in the patch passed.
          +1 asflicense 0m 28s The patch does not generate ASF License warnings.
          46m 56s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829439/HADOOP-13590.04.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c2c670cef956 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / e80386d
          Default Java 1.8.0_101
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10553/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10553/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 10s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 58s trunk passed +1 compile 8m 15s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 1m 0s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 31s trunk passed +1 javadoc 0m 50s trunk passed +1 mvninstall 0m 46s the patch passed +1 compile 8m 7s the patch passed +1 javac 8m 7s the patch passed +1 checkstyle 0m 27s the patch passed +1 mvnsite 1m 0s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 50s the patch passed +1 javadoc 0m 53s the patch passed +1 unit 12m 22s hadoop-common in the patch passed. +1 asflicense 0m 28s The patch does not generate ASF License warnings. 46m 56s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829439/HADOOP-13590.04.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c2c670cef956 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e80386d Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10553/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10553/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          drankye Kai Zheng added a comment -

          Thanks for proposing and working on this, Xiao Chen. Some quick comments:

          1. The new metric looks good. Maybe: renewalFailure => renewalFailures, also the method.

          +    @Metric("Renewal failures since last successful login")
          +    private long renewalFailure;
          

          2. The new test based on mock and faked things looks good. Is it possible to add some tests utilizing MiniKDC? For example, trying some cases like KDC being stopped then restarted. This would bring more confidence. Please feel free to open new one if you want to address this separately.

          Show
          drankye Kai Zheng added a comment - Thanks for proposing and working on this, Xiao Chen . Some quick comments: 1. The new metric looks good. Maybe: renewalFailure => renewalFailures, also the method. + @Metric( "Renewal failures since last successful login" ) + private long renewalFailure; 2. The new test based on mock and faked things looks good. Is it possible to add some tests utilizing MiniKDC? For example, trying some cases like KDC being stopped then restarted. This would bring more confidence. Please feel free to open new one if you want to address this separately.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Kai Zheng for reviewing!

          Patch 5 to address the comments. Renamed the param and updated the renewal test to be based on miniKDC. TestUGI all passes very fast, so I added a new test class for this test. Good news is we can remove the shouldSkipSleepForTests param, making the test more real.

          Also had minor javadoc/logging improvements.

          Show
          xiaochen Xiao Chen added a comment - Thanks Kai Zheng for reviewing! Patch 5 to address the comments. Renamed the param and updated the renewal test to be based on miniKDC. TestUGI all passes very fast, so I added a new test class for this test. Good news is we can remove the shouldSkipSleepForTests param, making the test more real. Also had minor javadoc/logging improvements.
          Hide
          drankye Kai Zheng added a comment -

          Thanks Xiao Chen for the quick action!

          Good news is we can remove the shouldSkipSleepForTests param, making the test more real.

          Ah, right! This is exactly what I wanted to say and wish to happen. As we all know, UGI codes pretty hard to maintain, would love to see some day all the test related codes separated out and cleaned up ...

          I will look at it closely late today.

          Show
          drankye Kai Zheng added a comment - Thanks Xiao Chen for the quick action! Good news is we can remove the shouldSkipSleepForTests param, making the test more real. Ah, right! This is exactly what I wanted to say and wish to happen. As we all know, UGI codes pretty hard to maintain, would love to see some day all the test related codes separated out and cleaned up ... I will look at it closely late today.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 7m 35s trunk passed
          +1 compile 7m 40s trunk passed
          +1 checkstyle 0m 26s trunk passed
          +1 mvnsite 1m 4s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 28s trunk passed
          +1 javadoc 0m 47s trunk passed
          +1 mvninstall 0m 40s the patch passed
          +1 compile 7m 43s the patch passed
          +1 javac 7m 43s the patch passed
          -0 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 1 new + 148 unchanged - 0 fixed = 149 total (was 148)
          +1 mvnsite 0m 59s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 39s the patch passed
          +1 javadoc 0m 48s the patch passed
          -1 unit 16m 40s hadoop-common in the patch failed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          50m 28s



          Reason Tests
          Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829736/HADOOP-13590.05.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 5c607a9c7555 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 537095d
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10564/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10564/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10564/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10564/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 7m 35s trunk passed +1 compile 7m 40s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 1m 4s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 28s trunk passed +1 javadoc 0m 47s trunk passed +1 mvninstall 0m 40s the patch passed +1 compile 7m 43s the patch passed +1 javac 7m 43s the patch passed -0 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 1 new + 148 unchanged - 0 fixed = 149 total (was 148) +1 mvnsite 0m 59s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 39s the patch passed +1 javadoc 0m 48s the patch passed -1 unit 16m 40s hadoop-common in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 50m 28s Reason Tests Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829736/HADOOP-13590.05.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 5c607a9c7555 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 537095d Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10564/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10564/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10564/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10564/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          drankye Kai Zheng added a comment -

          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 there?

          5. In the new test class TestUGIWithMiniKdc: I'm not sure if we need testUGI to doAs the call UserGroupInformation.loginUserFromSubject(loginSubject).

          +      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;
          +        }
          +      });
          
          Show
          drankye Kai Zheng added a comment - 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 there? 5. In the new test class TestUGIWithMiniKdc : I'm not sure if we need testUGI to doAs the call UserGroupInformation.loginUserFromSubject(loginSubject) . + 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 ; + } + });
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks for the review and feedback Kai!

          Patch 6 attached, addressed your comments, with the exception of:

          1. show now and renewalFailures values in the warning log?

          Updated the warning log and removed the debug log in the inner call.

          3. Could we return earlier at the beginning so we can avoid at least 2 level of indents and make the whole block more readable

          Agreed. Feels we should do this separately, created HADOOP-13641.

          4.Just a question. Any other exception than IOException could be thrown there?

          Checking line by line, I think only IOE and InterruptedException can be thrown.

          Show
          xiaochen Xiao Chen added a comment - Thanks for the review and feedback Kai! Patch 6 attached, addressed your comments, with the exception of: 1. show now and renewalFailures values in the warning log? Updated the warning log and removed the debug log in the inner call. 3. Could we return earlier at the beginning so we can avoid at least 2 level of indents and make the whole block more readable Agreed. Feels we should do this separately, created HADOOP-13641 . 4.Just a question. Any other exception than IOException could be thrown there? Checking line by line, I think only IOE and InterruptedException can be thrown.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 7m 32s trunk passed
          +1 compile 7m 57s trunk passed
          +1 checkstyle 0m 26s trunk passed
          +1 mvnsite 1m 1s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 23s trunk passed
          +1 javadoc 0m 45s trunk passed
          +1 mvninstall 0m 37s the patch passed
          +1 compile 7m 36s the patch passed
          +1 javac 7m 36s the patch passed
          +1 checkstyle 0m 27s the patch passed
          +1 mvnsite 1m 0s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 39s the patch passed
          +1 javadoc 0m 53s the patch passed
          -1 unit 17m 22s hadoop-common in the patch failed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          51m 7s



          Reason Tests
          Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829894/HADOOP-13590.06.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux e33cf06f9d6f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 8d619b4
          Default Java 1.8.0_101
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10573/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10573/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10573/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 7m 32s trunk passed +1 compile 7m 57s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 23s trunk passed +1 javadoc 0m 45s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 7m 36s the patch passed +1 javac 7m 36s the patch passed +1 checkstyle 0m 27s the patch passed +1 mvnsite 1m 0s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 39s the patch passed +1 javadoc 0m 53s the patch passed -1 unit 17m 22s hadoop-common in the patch failed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 51m 7s Reason Tests Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829894/HADOOP-13590.06.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux e33cf06f9d6f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 8d619b4 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10573/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10573/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10573/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          drankye Kai Zheng added a comment -

          Thanks Xiao Chen for the update! It looks good to me now. +1 from me.

          If any chance to process others' reviewing comments in the following, a very minor to address, tgt.getEndTime().getTime() repeated some times in the catch block.

          Show
          drankye Kai Zheng added a comment - Thanks Xiao Chen for the update! It looks good to me now. +1 from me. If any chance to process others' reviewing comments in the following, a very minor to address, tgt.getEndTime().getTime() repeated some times in the catch block.
          Hide
          xiaochen Xiao Chen added a comment -

          Thank you for the prompt reviews Kai!
          Will wait for other comments from the audience, and make sure the getEndTime get addressed in the final patch.

          Show
          xiaochen Xiao Chen added a comment - Thank you for the prompt reviews Kai! Will wait for other comments from the audience, and make sure the getEndTime get addressed in the final patch.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I don't think it's quite ready to put in, but getting v. close.

          1. is there any reason not to use a RetryPolicy here, rather than re-implement an algorithm?
            It could be a hard-coded choice of policy —but it is still stable code widely used.
          1. Test can probably import org.apache.hadoop.conf.Configuration rather than declare variables that way.
          2. I worry that test is going to be brittle about time. Why not let it spin for 60s, so it can handle a bit more clock jitter
            on test VMs.
          3. use KerberosUtil.getKrb5LoginModuleName() to get the login module name, instead of copy & paste.
          Show
          stevel@apache.org Steve Loughran added a comment - I don't think it's quite ready to put in, but getting v. close. is there any reason not to use a RetryPolicy here, rather than re-implement an algorithm? It could be a hard-coded choice of policy —but it is still stable code widely used. Test can probably import org.apache.hadoop.conf.Configuration rather than declare variables that way. I worry that test is going to be brittle about time. Why not let it spin for 60s, so it can handle a bit more clock jitter on test VMs. use KerberosUtil.getKrb5LoginModuleName() to get the login module name, instead of copy & paste.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks for the feedback, Steve Loughran.

          is there any reason not to use a RetryPolicy here

          Good question! The reason is the following:
          First of all, we definitely want exponential backoff, to prevent us causing ddos on kdc.

          In RetryPolicies, there is no RetryUpToMaxmumTimeWithProportinalSleep, and IMO the reason lacking one there, is it's not feasible/maintainable to calculate a maxRetries inline when invoking the base class ctor. It's eventually calculating a taylor series IIUC.

          In our case, we could calculate the maxRetries beforehand, then initialize a retryUpToMaximumCountWithProportionalSleep accordingly. That ends up in similar code to getNextTgtRenewalTime in the caller. Moreover, personally I feel the last retry before expiry could be helpful, otherwise the backoff will likely miss the end time.

          Test can probably import org.apache.hadoop.conf.Configuration rather than declare variables that way.

          Not really, there's a conflict with javax.security.auth.login.Configration. On a second thought I switched the two to make hadoop's Configuration the default.

          Other comments are addressed in patch 7.
          Regarding the test, having a real test is brittle and a bit time consuming (due to TICKET_RENEW_WINDOW), but having a fake test as Kai Zheng pointed out is.... fake. I don't have a strong option, but if it ends up spamming pre-commit, we may switch to the mock test after all.

          Show
          xiaochen Xiao Chen added a comment - Thanks for the feedback, Steve Loughran . is there any reason not to use a RetryPolicy here Good question! The reason is the following: First of all, we definitely want exponential backoff, to prevent us causing ddos on kdc. In RetryPolicies , there is no RetryUpToMaxmumTimeWithProportinalSleep , and IMO the reason lacking one there, is it's not feasible/maintainable to calculate a maxRetries inline when invoking the base class ctor. It's eventually calculating a taylor series IIUC. In our case, we could calculate the maxRetries beforehand, then initialize a retryUpToMaximumCountWithProportionalSleep accordingly. That ends up in similar code to getNextTgtRenewalTime in the caller. Moreover, personally I feel the last retry before expiry could be helpful, otherwise the backoff will likely miss the end time. Test can probably import org.apache.hadoop.conf.Configuration rather than declare variables that way. Not really, there's a conflict with javax.security.auth.login.Configration . On a second thought I switched the two to make hadoop's Configuration the default. Other comments are addressed in patch 7. Regarding the test, having a real test is brittle and a bit time consuming (due to TICKET_RENEW_WINDOW ), but having a fake test as Kai Zheng pointed out is.... fake. I don't have a strong option, but if it ends up spamming pre-commit, we may switch to the mock test after all.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 8m 33s trunk passed
          +1 compile 8m 26s trunk passed
          +1 checkstyle 0m 26s trunk passed
          +1 mvnsite 1m 8s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 30s trunk passed
          +1 javadoc 0m 50s trunk passed
          +1 mvninstall 0m 42s the patch passed
          +1 compile 7m 50s the patch passed
          +1 javac 7m 50s the patch passed
          -0 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 39 new + 148 unchanged - 0 fixed = 187 total (was 148)
          +1 mvnsite 1m 6s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 42s the patch passed
          +1 javadoc 0m 50s the patch passed
          -1 unit 8m 29s hadoop-common in the patch failed.
          +1 asflicense 0m 23s The patch does not generate ASF License warnings.
          44m 33s



          Reason Tests
          Failed junit tests hadoop.ha.TestZKFailoverController



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830092/HADOOP-13590.07.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c4dd3391b7ed 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 6e849cb
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10582/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10582/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10582/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10582/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 8m 33s trunk passed +1 compile 8m 26s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 1m 8s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 30s trunk passed +1 javadoc 0m 50s trunk passed +1 mvninstall 0m 42s the patch passed +1 compile 7m 50s the patch passed +1 javac 7m 50s the patch passed -0 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 39 new + 148 unchanged - 0 fixed = 187 total (was 148) +1 mvnsite 1m 6s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 42s the patch passed +1 javadoc 0m 50s the patch passed -1 unit 8m 29s hadoop-common in the patch failed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 44m 33s Reason Tests Failed junit tests hadoop.ha.TestZKFailoverController Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830092/HADOOP-13590.07.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c4dd3391b7ed 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6e849cb Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10582/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10582/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10582/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10582/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 7m 11s trunk passed
          +1 compile 7m 17s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 1m 22s trunk passed
          +1 javadoc 0m 45s trunk passed
          +1 mvninstall 0m 40s the patch passed
          +1 compile 7m 17s the patch passed
          +1 javac 7m 17s the patch passed
          -0 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 1 new + 148 unchanged - 0 fixed = 149 total (was 148)
          +1 mvnsite 0m 55s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 35s the patch passed
          +1 javadoc 0m 45s the patch passed
          +1 unit 8m 4s hadoop-common in the patch passed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          40m 10s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830116/HADOOP-13590.07.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 6419c2aa3f71 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 6eb700e
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10585/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10585/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10585/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 7m 11s trunk passed +1 compile 7m 17s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 22s trunk passed +1 javadoc 0m 45s trunk passed +1 mvninstall 0m 40s the patch passed +1 compile 7m 17s the patch passed +1 javac 7m 17s the patch passed -0 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 1 new + 148 unchanged - 0 fixed = 149 total (was 148) +1 mvnsite 0m 55s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 35s the patch passed +1 javadoc 0m 45s the patch passed +1 unit 8m 4s hadoop-common in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 40m 10s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830116/HADOOP-13590.07.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 6419c2aa3f71 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6eb700e Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10585/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10585/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10585/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Fixing checkstyle.
          Steve Loughran, please feel free to share your thoughts. Thank you.

          Show
          xiaochen Xiao Chen added a comment - Fixing checkstyle. Steve Loughran , please feel free to share your thoughts. Thank you.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 8m 54s trunk passed
          +1 compile 9m 2s trunk passed
          +1 checkstyle 0m 27s trunk passed
          +1 mvnsite 1m 15s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 47s trunk passed
          +1 javadoc 0m 43s trunk passed
          +1 mvninstall 0m 39s the patch passed
          +1 compile 7m 7s the patch passed
          +1 javac 7m 7s the patch passed
          +1 checkstyle 0m 24s the patch passed
          +1 mvnsite 0m 55s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 25s the patch passed
          +1 javadoc 0m 43s the patch passed
          +1 unit 7m 46s hadoop-common in the patch passed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          43m 37s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830558/HADOOP-13590.07.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 832738b28dde 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 1831be8
          Default Java 1.8.0_101
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10615/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10615/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 8m 54s trunk passed +1 compile 9m 2s trunk passed +1 checkstyle 0m 27s trunk passed +1 mvnsite 1m 15s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 47s trunk passed +1 javadoc 0m 43s trunk passed +1 mvninstall 0m 39s the patch passed +1 compile 7m 7s the patch passed +1 javac 7m 7s the patch passed +1 checkstyle 0m 24s the patch passed +1 mvnsite 0m 55s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 25s the patch passed +1 javadoc 0m 43s the patch passed +1 unit 7m 46s hadoop-common in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 43m 37s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830558/HADOOP-13590.07.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 832738b28dde 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 1831be8 Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10615/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10615/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Hi Steve Loughran,
          Any comments on this one? Thanks!

          Show
          xiaochen Xiao Chen added a comment - Hi Steve Loughran , Any comments on this one? Thanks!
          Hide
          andrew.wang Andrew Wang added a comment -

          Couple comments to try and push this forward:

          • I think the metric should be a MutableGauge instead of just a long.
          • Exponential back-off is supposed to be randomized within an exponentially increasing interval.
          • Regarding unit test flakiness, I'm okay with a unit test for just the retry logic, and then another unit test that makes sure it retries at all. IMO we should avoid sleeping in tests whenever possible, since unit tests are supposed to be quick to run.
          Show
          andrew.wang Andrew Wang added a comment - Couple comments to try and push this forward: I think the metric should be a MutableGauge instead of just a long. Exponential back-off is supposed to be randomized within an exponentially increasing interval. Regarding unit test flakiness, I'm okay with a unit test for just the retry logic, and then another unit test that makes sure it retries at all. IMO we should avoid sleeping in tests whenever possible, since unit tests are supposed to be quick to run.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Andrew Wang for looking at this! Patch 8 should address all the comments:

          unit test

          Now TestUGI tests the just the retry logic, and the new TestUGIWithMiniKdc tests the 'retries at all'.

          Exponential back-off

          Crossing your comment and Steve Loughran's comment about using RetryPolicy, I changed the code for retry-time calculation. Now it first calculates how many max retries could possibly be needed, then creates a ExponentialBackoffRetry object and delegates the calculation to it. This way we achieve the random interval + code reuse. The UGI code is (I think) harder to read though.

          Show
          xiaochen Xiao Chen added a comment - Thanks Andrew Wang for looking at this! Patch 8 should address all the comments: unit test Now TestUGI tests the just the retry logic, and the new TestUGIWithMiniKdc tests the 'retries at all'. Exponential back-off Crossing your comment and Steve Loughran 's comment about using RetryPolicy , I changed the code for retry-time calculation. Now it first calculates how many max retries could possibly be needed, then creates a ExponentialBackoffRetry object and delegates the calculation to it. This way we achieve the random interval + code reuse. The UGI code is (I think) harder to read though.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 23s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 6m 42s trunk passed
          +1 compile 6m 51s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 0m 55s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 1m 20s trunk passed
          +1 javadoc 0m 41s trunk passed
          +1 mvninstall 0m 37s the patch passed
          +1 compile 6m 49s the patch passed
          +1 javac 6m 49s the patch passed
          -0 checkstyle 0m 25s hadoop-common-project/hadoop-common: The patch generated 2 new + 145 unchanged - 0 fixed = 147 total (was 145)
          +1 mvnsite 0m 54s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 25s the patch passed
          +1 javadoc 0m 42s the patch passed
          +1 unit 8m 13s hadoop-common in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          38m 37s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12836288/HADOOP-13590.08.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux d19d7cb36776 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 7ba74be
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10942/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10942/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10942/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 23s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 6m 42s trunk passed +1 compile 6m 51s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 55s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 20s trunk passed +1 javadoc 0m 41s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 6m 49s the patch passed +1 javac 6m 49s the patch passed -0 checkstyle 0m 25s hadoop-common-project/hadoop-common: The patch generated 2 new + 145 unchanged - 0 fixed = 147 total (was 145) +1 mvnsite 0m 54s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 25s the patch passed +1 javadoc 0m 42s the patch passed +1 unit 8m 13s hadoop-common in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 38m 37s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12836288/HADOOP-13590.08.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux d19d7cb36776 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 7ba74be Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10942/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10942/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10942/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I'm looking at getMaxTgtRenewalRetryCount and not sure I understand it. I'm confident I could work out what it does given time, but it's not immediately obvious. What does it do? And could some comments in the code describe that

          Test-wise, I've added support for more backoff in tests that wait; look in LambdaTestUtils.

          I also see that the code to set up a javax.security.auth.login.Configuration is surfacing again...there are a fair few copies of this code around (I know one in the registry tests), all of which have those same hacks for IBM JVMs, etc. Which makes it a maintenance pain. Could we think about coalescing them into one or two utility methods somewhere?

          Show
          stevel@apache.org Steve Loughran added a comment - I'm looking at getMaxTgtRenewalRetryCount and not sure I understand it. I'm confident I could work out what it does given time, but it's not immediately obvious. What does it do? And could some comments in the code describe that Test-wise, I've added support for more backoff in tests that wait; look in LambdaTestUtils. I also see that the code to set up a javax.security.auth.login.Configuration is surfacing again...there are a fair few copies of this code around (I know one in the registry tests), all of which have those same hacks for IBM JVMs, etc. Which makes it a maintenance pain. Could we think about coalescing them into one or two utility methods somewhere?
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Steve Loughran for the prompt review!
          Good point on getMaxTgtRenewalRetryCount, on a second thought I think it can be eliminated, so the retry policy goes to Int.MAX_VALUE and we simply check it against the end time. Currently it's only making sure we can create the RetryPolicy with correct maxRetries. Will do that in the next patch, and add comments.

          Test-wise, I've added support for more backoff in tests that wait; look in LambdaTestUtils.

          Thanks for the good work, let me try replace the GenericTestUtil usage with it.

          I also see that the code to set up a javax.security.auth.login.Configuration is surfacing again...

          See my comment above, it's due to conflicting class name Configuration in hadoop and in javax. I guess we'll have to explicitly define one way or the other.
          Happy to wrap up a utility function to clean up all IBM hacks etc., I propose to create a separate jira to limit scope of this one. Please let me know if you feel otherwise.

          Show
          xiaochen Xiao Chen added a comment - Thanks Steve Loughran for the prompt review! Good point on getMaxTgtRenewalRetryCount , on a second thought I think it can be eliminated, so the retry policy goes to Int.MAX_VALUE and we simply check it against the end time. Currently it's only making sure we can create the RetryPolicy with correct maxRetries. Will do that in the next patch, and add comments. Test-wise, I've added support for more backoff in tests that wait; look in LambdaTestUtils. Thanks for the good work, let me try replace the GenericTestUtil usage with it. I also see that the code to set up a javax.security.auth.login.Configuration is surfacing again... See my comment above , it's due to conflicting class name Configuration in hadoop and in javax. I guess we'll have to explicitly define one way or the other. Happy to wrap up a utility function to clean up all IBM hacks etc., I propose to create a separate jira to limit scope of this one. Please let me know if you feel otherwise.
          Hide
          xiaochen Xiao Chen added a comment -

          Patch 9 to address the comments. I find the lambda wait() easier to use than GenericTestUtil#waitFor. Thank you Steve.

          Created HADOOP-13785 for the clean up.

          Show
          xiaochen Xiao Chen added a comment - Patch 9 to address the comments. I find the lambda wait() easier to use than GenericTestUtil#waitFor . Thank you Steve. Created HADOOP-13785 for the clean up.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 21s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 6m 43s trunk passed
          +1 compile 6m 54s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 18s trunk passed
          +1 javadoc 0m 44s trunk passed
          +1 mvninstall 0m 36s the patch passed
          +1 compile 7m 15s the patch passed
          +1 javac 7m 15s the patch passed
          -0 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 6 new + 145 unchanged - 0 fixed = 151 total (was 145)
          +1 mvnsite 0m 56s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 1m 33s hadoop-common-project/hadoop-common generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 0m 44s the patch passed
          +1 unit 9m 8s hadoop-common in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          40m 10s



          Reason Tests
          FindBugs module:hadoop-common-project/hadoop-common
            Redundant nullcheck of tgt, which is known to be non-null in org.apache.hadoop.security.UserGroupInformation$1.run() Redundant null check at UserGroupInformation.java:is known to be non-null in org.apache.hadoop.security.UserGroupInformation$1.run() Redundant null check at UserGroupInformation.java:[line 1011]



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12836501/HADOOP-13590.09.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 3c650cd37659 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / fc2b69e
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10959/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10959/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-common.html
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10959/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10959/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 21s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 6m 43s trunk passed +1 compile 6m 54s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 18s trunk passed +1 javadoc 0m 44s trunk passed +1 mvninstall 0m 36s the patch passed +1 compile 7m 15s the patch passed +1 javac 7m 15s the patch passed -0 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 6 new + 145 unchanged - 0 fixed = 151 total (was 145) +1 mvnsite 0m 56s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 1m 33s hadoop-common-project/hadoop-common generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 0m 44s the patch passed +1 unit 9m 8s hadoop-common in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 40m 10s Reason Tests FindBugs module:hadoop-common-project/hadoop-common   Redundant nullcheck of tgt, which is known to be non-null in org.apache.hadoop.security.UserGroupInformation$1.run() Redundant null check at UserGroupInformation.java:is known to be non-null in org.apache.hadoop.security.UserGroupInformation$1.run() Redundant null check at UserGroupInformation.java: [line 1011] Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12836501/HADOOP-13590.09.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 3c650cd37659 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / fc2b69e Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10959/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/10959/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-common.html Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10959/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10959/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Seems my IDE was confused on indentation by the lambda. This should fix it.

          Show
          xiaochen Xiao Chen added a comment - Seems my IDE was confused on indentation by the lambda. This should fix it.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 8m 49s trunk passed
          +1 compile 7m 3s trunk passed
          +1 checkstyle 0m 25s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 20s trunk passed
          +1 javadoc 0m 43s trunk passed
          +1 mvninstall 0m 37s the patch passed
          +1 compile 6m 53s the patch passed
          +1 javac 6m 53s the patch passed
          -0 checkstyle 0m 25s hadoop-common-project/hadoop-common: The patch generated 5 new + 145 unchanged - 0 fixed = 150 total (was 145)
          +1 mvnsite 0m 55s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 28s the patch passed
          +1 javadoc 0m 41s the patch passed
          +1 unit 7m 56s hadoop-common in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          40m 39s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12836504/HADOOP-13590.09.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 043e4538fbfa 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / cb5cc0d
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10960/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10960/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10960/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 8m 49s trunk passed +1 compile 7m 3s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 20s trunk passed +1 javadoc 0m 43s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 6m 53s the patch passed +1 javac 6m 53s the patch passed -0 checkstyle 0m 25s hadoop-common-project/hadoop-common: The patch generated 5 new + 145 unchanged - 0 fixed = 150 total (was 145) +1 mvnsite 0m 55s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 28s the patch passed +1 javadoc 0m 41s the patch passed +1 unit 7m 56s hadoop-common in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 40m 39s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12836504/HADOOP-13590.09.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 043e4538fbfa 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / cb5cc0d Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10960/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10960/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10960/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          chesktyle is false alarm, I see it had similar confusions in HADOOP-13716.

          Show
          xiaochen Xiao Chen added a comment - chesktyle is false alarm, I see it had similar confusions in HADOOP-13716 .
          Hide
          andrew.wang Andrew Wang added a comment -

          LGTM, +1 pending:

          • If this is intended for backport to branch-2, we can't use lambdas since branch-2 is still on JDK7.
          • Tiniest of nits, would like a space in this comment before "client": "//client login"
          Show
          andrew.wang Andrew Wang added a comment - LGTM, +1 pending: If this is intended for backport to branch-2, we can't use lambdas since branch-2 is still on JDK7. Tiniest of nits, would like a space in this comment before "client": "//client login"
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I so wish we could use L-exp in branch-2...for now keep the await() call, just use an anon class, at least for the branch-2 version, leaving trunk as is.

          Given that the assertTrue calls are preceded by log@Info calls, how about building a string which is then logged and used as the text in the asserts?

          Show
          stevel@apache.org Steve Loughran added a comment - I so wish we could use L-exp in branch-2...for now keep the await() call, just use an anon class, at least for the branch-2 version, leaving trunk as is. Given that the assertTrue calls are preceded by log@Info calls, how about building a string which is then logged and used as the text in the asserts?
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Andrew and Steve.

          Patch 10 to:

          • Added the space in TestUGIWithMiniKdc
          • Added to string to assertion message. Extracted a method to make this cleaner.

          Will provide a branch-2 patch shortly.

          Show
          xiaochen Xiao Chen added a comment - Thanks Andrew and Steve. Patch 10 to: Added the space in TestUGIWithMiniKdc Added to string to assertion message. Extracted a method to make this cleaner. Will provide a branch-2 patch shortly.
          Hide
          xiaochen Xiao Chen added a comment -

          TestUGIWithMiniKdc wouldn't work well in Branch-2. This is due to kerby doesn't allow less-than-6-minutes ticket lifetime. This was reported before .

          The error is:

          javax.security.auth.login.LoginException: Requested start time is later than end time (11) - Requested start time is later than end time
          
          	at com.sun.security.auth.module.Krb5LoginModule.attemptAuthentication(Krb5LoginModule.java:804)
          	at com.sun.security.auth.module.Krb5LoginModule.login(Krb5LoginModule.java:617)
          	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
          	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
          	at java.lang.reflect.Method.invoke(Method.java:497)
          	at javax.security.auth.login.LoginContext.invoke(LoginContext.java:755)
          	at javax.security.auth.login.LoginContext.access$000(LoginContext.java:195)
          	at javax.security.auth.login.LoginContext$4.run(LoginContext.java:682)
          	at javax.security.auth.login.LoginContext$4.run(LoginContext.java:680)
          	at java.security.AccessController.doPrivileged(Native Method)
          	at javax.security.auth.login.LoginContext.invokePriv(LoginContext.java:680)
          	at javax.security.auth.login.LoginContext.login(LoginContext.java:587)
          	at org.apache.hadoop.security.TestUGIWithMiniKdc.testAutoRenewalThreadRetryWithKdc(TestUGIWithMiniKdc.java:126)
          	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
          	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
          	at java.lang.reflect.Method.invoke(Method.java:497)
          	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
          	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
          	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
          	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
          	at org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74)
          Caused by: KrbException: Requested start time is later than end time (11) - Requested start time is later than end time
          	at sun.security.krb5.KrbAsRep.<init>(KrbAsRep.java:82)
          	at sun.security.krb5.KrbAsReqBuilder.send(KrbAsReqBuilder.java:316)
          	at sun.security.krb5.KrbAsReqBuilder.action(KrbAsReqBuilder.java:361)
          	at com.sun.security.auth.module.Krb5LoginModule.attemptAuthentication(Krb5LoginModule.java:776)
          	... 22 more
          Caused by: KrbException: Identifier doesn't match expected value (906)
          	at sun.security.krb5.internal.KDCRep.init(KDCRep.java:140)
          	at sun.security.krb5.internal.ASRep.init(ASRep.java:64)
          	at sun.security.krb5.internal.ASRep.<init>(ASRep.java:59)
          	at sun.security.krb5.KrbAsRep.<init>(KrbAsRep.java:60)
          	... 25 more
          

          I have manually verified the backported test to pass with MAX_TICKET_LIFETIME set to 360000. But propose not to include the test for branch-2 to save execution time.

          Show
          xiaochen Xiao Chen added a comment - TestUGIWithMiniKdc wouldn't work well in Branch-2. This is due to kerby doesn't allow less-than-6-minutes ticket lifetime. This was reported before . The error is: javax.security.auth.login.LoginException: Requested start time is later than end time (11) - Requested start time is later than end time at com.sun.security.auth.module.Krb5LoginModule.attemptAuthentication(Krb5LoginModule.java:804) at com.sun.security.auth.module.Krb5LoginModule.login(Krb5LoginModule.java:617) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:497) at javax.security.auth.login.LoginContext.invoke(LoginContext.java:755) at javax.security.auth.login.LoginContext.access$000(LoginContext.java:195) at javax.security.auth.login.LoginContext$4.run(LoginContext.java:682) at javax.security.auth.login.LoginContext$4.run(LoginContext.java:680) at java.security.AccessController.doPrivileged(Native Method) at javax.security.auth.login.LoginContext.invokePriv(LoginContext.java:680) at javax.security.auth.login.LoginContext.login(LoginContext.java:587) at org.apache.hadoop.security.TestUGIWithMiniKdc.testAutoRenewalThreadRetryWithKdc(TestUGIWithMiniKdc.java:126) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:497) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74) Caused by: KrbException: Requested start time is later than end time (11) - Requested start time is later than end time at sun.security.krb5.KrbAsRep.<init>(KrbAsRep.java:82) at sun.security.krb5.KrbAsReqBuilder.send(KrbAsReqBuilder.java:316) at sun.security.krb5.KrbAsReqBuilder.action(KrbAsReqBuilder.java:361) at com.sun.security.auth.module.Krb5LoginModule.attemptAuthentication(Krb5LoginModule.java:776) ... 22 more Caused by: KrbException: Identifier doesn't match expected value (906) at sun.security.krb5.internal.KDCRep.init(KDCRep.java:140) at sun.security.krb5.internal.ASRep.init(ASRep.java:64) at sun.security.krb5.internal.ASRep.<init>(ASRep.java:59) at sun.security.krb5.KrbAsRep.<init>(KrbAsRep.java:60) ... 25 more I have manually verified the backported test to pass with MAX_TICKET_LIFETIME set to 360000 . But propose not to include the test for branch-2 to save execution time.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 8m 7s trunk passed
          +1 compile 8m 17s trunk passed
          +1 checkstyle 0m 27s trunk passed
          +1 mvnsite 1m 6s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 1m 38s trunk passed
          +1 javadoc 0m 46s trunk passed
          +1 mvninstall 0m 47s the patch passed
          +1 compile 8m 4s the patch passed
          +1 javac 8m 4s the patch passed
          -0 checkstyle 0m 25s hadoop-common-project/hadoop-common: The patch generated 5 new + 145 unchanged - 0 fixed = 150 total (was 145)
          +1 mvnsite 0m 56s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 29s the patch passed
          +1 javadoc 0m 43s the patch passed
          -1 unit 7m 0s hadoop-common in the patch failed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          42m 17s



          Reason Tests
          Failed junit tests hadoop.net.TestDNS



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12837218/HADOOP-13590.10.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux f35a0747a98b 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / abfc15d
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10994/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10994/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10994/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10994/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 8m 7s trunk passed +1 compile 8m 17s trunk passed +1 checkstyle 0m 27s trunk passed +1 mvnsite 1m 6s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 38s trunk passed +1 javadoc 0m 46s trunk passed +1 mvninstall 0m 47s the patch passed +1 compile 8m 4s the patch passed +1 javac 8m 4s the patch passed -0 checkstyle 0m 25s hadoop-common-project/hadoop-common: The patch generated 5 new + 145 unchanged - 0 fixed = 150 total (was 145) +1 mvnsite 0m 56s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 29s the patch passed +1 javadoc 0m 43s the patch passed -1 unit 7m 0s hadoop-common in the patch failed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 42m 17s Reason Tests Failed junit tests hadoop.net.TestDNS Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12837218/HADOOP-13590.10.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux f35a0747a98b 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / abfc15d Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10994/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10994/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10994/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10994/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 6m 36s branch-2 passed
          +1 compile 5m 38s branch-2 passed with JDK v1.8.0_101
          +1 compile 6m 29s branch-2 passed with JDK v1.7.0_111
          +1 checkstyle 0m 27s branch-2 passed
          +1 mvnsite 0m 57s branch-2 passed
          +1 mvneclipse 0m 16s branch-2 passed
          +1 findbugs 1m 39s branch-2 passed
          +1 javadoc 0m 46s branch-2 passed with JDK v1.8.0_101
          +1 javadoc 0m 56s branch-2 passed with JDK v1.7.0_111
          +1 mvninstall 0m 40s the patch passed
          +1 compile 6m 51s the patch passed with JDK v1.8.0_101
          +1 javac 6m 51s the patch passed
          +1 compile 7m 31s the patch passed with JDK v1.7.0_111
          +1 javac 7m 31s the patch passed
          +1 checkstyle 0m 29s the patch passed
          +1 mvnsite 1m 4s the patch passed
          +1 mvneclipse 0m 16s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 2m 8s the patch passed
          +1 javadoc 0m 45s the patch passed with JDK v1.8.0_101
          +1 javadoc 0m 58s the patch passed with JDK v1.7.0_111
          +1 unit 8m 56s hadoop-common in the patch passed with JDK v1.7.0_111.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          64m 58s



          Reason Tests
          JDK v1.8.0_101 Failed junit tests hadoop.ha.TestZKFailoverController



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:b59b8b7
          JIRA Issue HADOOP-13590
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12837233/HADOOP-13590.branch-2.01.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 6eb5bfb12ae4 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision branch-2 / 0b36dcd
          Default Java 1.7.0_111
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111
          findbugs v3.0.0
          JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10995/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10995/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 36s branch-2 passed +1 compile 5m 38s branch-2 passed with JDK v1.8.0_101 +1 compile 6m 29s branch-2 passed with JDK v1.7.0_111 +1 checkstyle 0m 27s branch-2 passed +1 mvnsite 0m 57s branch-2 passed +1 mvneclipse 0m 16s branch-2 passed +1 findbugs 1m 39s branch-2 passed +1 javadoc 0m 46s branch-2 passed with JDK v1.8.0_101 +1 javadoc 0m 56s branch-2 passed with JDK v1.7.0_111 +1 mvninstall 0m 40s the patch passed +1 compile 6m 51s the patch passed with JDK v1.8.0_101 +1 javac 6m 51s the patch passed +1 compile 7m 31s the patch passed with JDK v1.7.0_111 +1 javac 7m 31s the patch passed +1 checkstyle 0m 29s the patch passed +1 mvnsite 1m 4s the patch passed +1 mvneclipse 0m 16s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 8s the patch passed +1 javadoc 0m 45s the patch passed with JDK v1.8.0_101 +1 javadoc 0m 58s the patch passed with JDK v1.7.0_111 +1 unit 8m 56s hadoop-common in the patch passed with JDK v1.7.0_111. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 64m 58s Reason Tests JDK v1.8.0_101 Failed junit tests hadoop.ha.TestZKFailoverController Subsystem Report/Notes Docker Image:yetus/hadoop:b59b8b7 JIRA Issue HADOOP-13590 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12837233/HADOOP-13590.branch-2.01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 6eb5bfb12ae4 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision branch-2 / 0b36dcd Default Java 1.7.0_111 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111 findbugs v3.0.0 JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10995/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10995/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Test failures on both trunk and branch-2 look unrelated and passed locally. Trunk's checkstyle is the same old to be overruled.

          Appreciate the reviews. Thanks.

          Show
          xiaochen Xiao Chen added a comment - Test failures on both trunk and branch-2 look unrelated and passed locally. Trunk's checkstyle is the same old to be overruled. Appreciate the reviews. Thanks.
          Hide
          xiaochen Xiao Chen added a comment -

          Andrew had a +1 pending earlier, but patch 10 also addressed Steve's comment in the test. So I'm not sure: is that +1 pending still valid?

          Thank you both again for the reviews.

          Show
          xiaochen Xiao Chen added a comment - Andrew had a +1 pending earlier, but patch 10 also addressed Steve's comment in the test. So I'm not sure: is that +1 pending still valid? Thank you both again for the reviews.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          LGTM

          +1

          Show
          stevel@apache.org Steve Loughran added a comment - LGTM +1
          Hide
          xiaochen Xiao Chen added a comment -

          Committed this to trunk, branch-2 and branch-2.8.

          Thanks a lot for the reviews Steve Loughran, Kai Zheng and Andrew Wang!

          Show
          xiaochen Xiao Chen added a comment - Committed this to trunk, branch-2 and branch-2.8. Thanks a lot for the reviews Steve Loughran , Kai Zheng and Andrew Wang !
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10801 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10801/)
          HADOOP-13590. Retry until TGT expires even if the UGI renewal thread (xiao: rev 367c3d41217728c2e61252c5a5235e5bc1f9822f)

          • (add) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUGIWithMiniKdc.java
          • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10801 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10801/ ) HADOOP-13590 . Retry until TGT expires even if the UGI renewal thread (xiao: rev 367c3d41217728c2e61252c5a5235e5bc1f9822f) (add) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUGIWithMiniKdc.java (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java

            People

            • Assignee:
              xiaochen Xiao Chen
              Reporter:
              xiaochen Xiao Chen
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development