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

Update UGI#spawnAutoRenewalThreadForUserCreds to reduce indentation

    Details

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

      Description

      From Kai Zheng's comment in HADOOP-13590:

      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 ...
      ...
      
      1. HADOOP-13641.1.patch
        4 kB
        Huafeng Wang
      2. HADOOP-13641.2.patch
        4 kB
        Huafeng Wang
      3. HADOOP-13641-branch-2.01.patch
        4 kB
        Huafeng Wang

        Activity

        Hide
        HuafengWang Huafeng Wang added a comment -

        I attached my first patch available for this and please help to review it.

        The patch just extract the if statement and return at beginning. There is no logical change in the thread implementation.

        Show
        HuafengWang Huafeng Wang added a comment - I attached my first patch available for this and please help to review it. The patch just extract the if statement and return at beginning. There is no logical change in the thread implementation.
        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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 mvninstall 7m 7s trunk passed
        +1 compile 7m 22s trunk passed
        +1 checkstyle 0m 25s trunk passed
        +1 mvnsite 1m 1s trunk passed
        +1 mvneclipse 0m 12s trunk passed
        +1 findbugs 1m 26s trunk passed
        +1 javadoc 0m 46s trunk passed
        +1 mvninstall 0m 39s the patch passed
        +1 compile 7m 11s the patch passed
        +1 javac 7m 11s the patch passed
        -0 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 4 new + 98 unchanged - 0 fixed = 102 total (was 98)
        +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 the patch passed
        +1 javadoc 0m 47s the patch passed
        +1 unit 10m 42s hadoop-common in the patch passed.
        +1 asflicense 0m 23s The patch does not generate ASF License warnings.
        42m 51s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HADOOP-13641
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829989/HADOOP-13641.1.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 41319218a512 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 / e5ef51e
        Default Java 1.8.0_101
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10579/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10579/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10579/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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 7m 7s trunk passed +1 compile 7m 22s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 26s trunk passed +1 javadoc 0m 46s trunk passed +1 mvninstall 0m 39s the patch passed +1 compile 7m 11s the patch passed +1 javac 7m 11s the patch passed -0 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 4 new + 98 unchanged - 0 fixed = 102 total (was 98) +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 the patch passed +1 javadoc 0m 47s the patch passed +1 unit 10m 42s hadoop-common in the patch passed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 42m 51s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13641 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829989/HADOOP-13641.1.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 41319218a512 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 / e5ef51e Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10579/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10579/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10579/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 -

        Hi Huafeng Wang,

        Can you address the checking styles? And also, would be good to have some line breaks before:

            //spawn thread only if we have kerb credentials
            Thread t = new Thread(new Runnable() {
        
        Show
        drankye Kai Zheng added a comment - Hi Huafeng Wang , Can you address the checking styles? And also, would be good to have some line breaks before: //spawn thread only if we have kerb credentials Thread t = new Thread ( new Runnable () {
        Hide
        drankye Kai Zheng added a comment -

        Oops, I thought the logic isn't correct. Previously if isKeytab is true, it will not spawn the thread and return.

        -    if (isSecurityEnabled()) {
        -      //spawn thread only if we have kerb credentials
        -      if (user.getAuthenticationMethod() == AuthenticationMethod.KERBEROS &&
        -          !isKeytab) {
        -        Thread t = new Thread(new Runnable() {
        ...
        +    if (!isSecurityEnabled()
        +      || user.getAuthenticationMethod() != AuthenticationMethod.KERBEROS
        +      || !isKeytab) {
        +      return;
        +    }
        
        Show
        drankye Kai Zheng added a comment - Oops, I thought the logic isn't correct. Previously if isKeytab is true, it will not spawn the thread and return. - if (isSecurityEnabled()) { - //spawn thread only if we have kerb credentials - if (user.getAuthenticationMethod() == AuthenticationMethod.KERBEROS && - !isKeytab) { - Thread t = new Thread ( new Runnable () { ... + if (!isSecurityEnabled() + || user.getAuthenticationMethod() != AuthenticationMethod.KERBEROS + || !isKeytab) { + return ; + }
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 25s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 mvninstall 7m 3s trunk passed
        +1 compile 6m 50s trunk passed
        +1 checkstyle 0m 24s trunk passed
        +1 mvnsite 0m 57s trunk passed
        +1 mvneclipse 0m 13s trunk passed
        +1 findbugs 1m 20s trunk passed
        +1 javadoc 0m 41s trunk passed
        +1 mvninstall 0m 36s the patch passed
        +1 compile 6m 47s the patch passed
        +1 javac 6m 47s the patch passed
        +1 checkstyle 0m 24s the patch passed
        +1 mvnsite 0m 53s the patch passed
        +1 mvneclipse 0m 13s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 28s the patch passed
        +1 javadoc 0m 42s the patch passed
        +1 unit 8m 19s hadoop-common in the patch passed.
        +1 asflicense 0m 22s The patch does not generate ASF License warnings.
        39m 6s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HADOOP-13641
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12832326/HADOOP-13641.2.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux b739f0878d61 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 / 4d10621
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10714/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10714/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 25s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 7m 3s trunk passed +1 compile 6m 50s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 57s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 20s trunk passed +1 javadoc 0m 41s trunk passed +1 mvninstall 0m 36s the patch passed +1 compile 6m 47s the patch passed +1 javac 6m 47s the patch passed +1 checkstyle 0m 24s the patch passed +1 mvnsite 0m 53s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 28s the patch passed +1 javadoc 0m 42s the patch passed +1 unit 8m 19s hadoop-common in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 39m 6s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13641 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12832326/HADOOP-13641.2.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux b739f0878d61 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 / 4d10621 Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10714/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10714/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        HuafengWang Huafeng Wang added a comment -

        Hi Kai Zheng,
        Thanks for your review and I uploaded a new patch which fixed the check style and the issue you mentioned. Please help to review it.

        Show
        HuafengWang Huafeng Wang added a comment - Hi Kai Zheng , Thanks for your review and I uploaded a new patch which fixed the check style and the issue you mentioned. Please help to review it.
        Hide
        drankye Kai Zheng added a comment -

        The latest patch LGTM and +1. Thanks Huafeng for the update.

        Show
        drankye Kai Zheng added a comment - The latest patch LGTM and +1. Thanks Huafeng for the update.
        Hide
        drankye Kai Zheng added a comment -

        Hi Huafeng Wang,

        Could you also provide a patch for branch-2 since there is conflict? Thanks!

        Show
        drankye Kai Zheng added a comment - Hi Huafeng Wang , Could you also provide a patch for branch-2 since there is conflict? Thanks!
        Hide
        HuafengWang Huafeng Wang added a comment -

        Hi Kai Zheng, I just uploaded another patch which is based on branch-2 and I have confirmed that there won't be conflict on branch-2.8 either.

        Show
        HuafengWang Huafeng Wang added a comment - Hi Kai Zheng , I just uploaded another patch which is based on branch-2 and I have confirmed that there won't be conflict on branch-2.8 either.
        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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 mvninstall 6m 39s branch-2 passed
        +1 compile 5m 34s 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 28s branch-2 passed
        +1 mvnsite 0m 58s branch-2 passed
        +1 mvneclipse 0m 16s branch-2 passed
        +1 findbugs 1m 39s branch-2 passed
        +1 javadoc 0m 45s 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 5m 26s the patch passed with JDK v1.8.0_101
        +1 javac 5m 26s the patch passed
        +1 compile 6m 33s the patch passed with JDK v1.7.0_111
        +1 javac 6m 33s the patch passed
        +1 checkstyle 0m 26s the patch passed
        +1 mvnsite 0m 58s the patch passed
        +1 mvneclipse 0m 16s the patch passed
        -1 whitespace 0m 0s The patch has 47 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
        +1 findbugs 1m 53s the patch passed
        +1 javadoc 0m 45s the patch passed with JDK v1.8.0_101
        +1 javadoc 0m 57s the patch passed with JDK v1.7.0_111
        +1 unit 8m 16s hadoop-common in the patch passed with JDK v1.7.0_111.
        +1 asflicense 0m 22s The patch does not generate ASF License warnings.
        60m 53s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:b59b8b7
        JIRA Issue HADOOP-13641
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12832339/HADOOP-13641-branch-2.01.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux df54e88cbb9d 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 / 5c96ef3
        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
        whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/10716/artifact/patchprocess/whitespace-eol.txt
        JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10716/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10716/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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 6m 39s branch-2 passed +1 compile 5m 34s 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 28s branch-2 passed +1 mvnsite 0m 58s branch-2 passed +1 mvneclipse 0m 16s branch-2 passed +1 findbugs 1m 39s branch-2 passed +1 javadoc 0m 45s 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 5m 26s the patch passed with JDK v1.8.0_101 +1 javac 5m 26s the patch passed +1 compile 6m 33s the patch passed with JDK v1.7.0_111 +1 javac 6m 33s the patch passed +1 checkstyle 0m 26s the patch passed +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 16s the patch passed -1 whitespace 0m 0s The patch has 47 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 findbugs 1m 53s the patch passed +1 javadoc 0m 45s the patch passed with JDK v1.8.0_101 +1 javadoc 0m 57s the patch passed with JDK v1.7.0_111 +1 unit 8m 16s hadoop-common in the patch passed with JDK v1.7.0_111. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 60m 53s Subsystem Report/Notes Docker Image:yetus/hadoop:b59b8b7 JIRA Issue HADOOP-13641 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12832339/HADOOP-13641-branch-2.01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux df54e88cbb9d 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 / 5c96ef3 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 whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/10716/artifact/patchprocess/whitespace-eol.txt JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10716/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10716/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        HuafengWang Huafeng Wang added a comment -

        Hi Kai Zheng, the white space issue is irrelevant to the patch.

        Show
        HuafengWang Huafeng Wang added a comment - Hi Kai Zheng , the white space issue is irrelevant to the patch.
        Hide
        drankye Kai Zheng added a comment -

        Thanks for the contribution, Huafeng Wang. Committed to trunk, branch-2 and branch-2.8

        Show
        drankye Kai Zheng added a comment - Thanks for the contribution, Huafeng Wang . Committed to trunk, branch-2 and branch-2.8
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10576 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10576/)
        HADOOP-13641. Update UGI#spawnAutoRenewalThreadForUserCreds to reduce (kai.zheng: rev 3d59b18d49d98a293ae14c5b89d515ef83cc4ff7)

        • (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 #10576 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10576/ ) HADOOP-13641 . Update UGI#spawnAutoRenewalThreadForUserCreds to reduce (kai.zheng: rev 3d59b18d49d98a293ae14c5b89d515ef83cc4ff7) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java

          People

          • Assignee:
            HuafengWang Huafeng Wang
            Reporter:
            xiaochen Xiao Chen
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development