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

Azure: Add Kerberos and Delegation token support to WASB client.

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.8.0
    • Fix Version/s: 2.9.0, 3.0.0-alpha4
    • Component/s: fs/azure
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      Current implementation of Azure storage client for Hadoop (WASB) does not support Kerberos Authentication and FileSystem authorization, which makes it unusable in secure environments with multi user setup.
      To make WASB client more suitable to run in Secure environments, there are 2 initiatives under way for providing the authorization (HADOOP-13930) and fine grained access control (HADOOP-13863) support.

      This JIRA is created to add Kerberos and delegation token support to WASB client to fetch Azure Storage SAS keys (from Remote service as discussed in HADOOP-13863), which provides fine grained timed access to containers and blobs.
      For delegation token management, the proposal is it use the same REST service which being used to generate the SAS Keys.

      1. HADOOP-13945.1.patch
        55 kB
        Santhosh G Nayak
      2. HADOOP-13945.10.patch
        49 kB
        Mingliang Liu
      3. HADOOP-13945.11.patch
        49 kB
        Santhosh G Nayak
      4. HADOOP-13945.2.patch
        30 kB
        Santhosh G Nayak
      5. HADOOP-13945.3.patch
        70 kB
        Santhosh G Nayak
      6. HADOOP-13945.4.patch
        71 kB
        Santhosh G Nayak
      7. HADOOP-13945.5.patch
        71 kB
        Santhosh G Nayak
      8. HADOOP-13945.6.patch
        47 kB
        Santhosh G Nayak
      9. HADOOP-13945.7.patch
        48 kB
        Santhosh G Nayak
      10. HADOOP-13945.8.patch
        48 kB
        Santhosh G Nayak
      11. HADOOP-13945.9.patch
        49 kB
        Mingliang Liu

        Issue Links

          Activity

          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11430 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11430/)
          HADOOP-13945. Azure: Add Kerberos and Delegation token support to WASB (liuml07: rev 8e15e240597f821968e14893eabfea39815de207)

          • (edit) hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/MockWasbAuthorizerImpl.java
          • (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/RemoteSASKeyGeneratorImpl.java
          • (add) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/security/WasbDelegationTokenIdentifier.java
          • (add) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/security/package-info.java
          • (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/WasbAuthorizerInterface.java
          • (add) hadoop-tools/hadoop-azure/src/main/resources/META-INF/services/org.apache.hadoop.security.token.TokenIdentifier
          • (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/RemoteWasbAuthorizerImpl.java
          • (add) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/security/SecurityUtils.java
          • (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/SecureStorageInterfaceImpl.java
          • (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
          • (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
          • (add) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/security/Constants.java
          • (add) hadoop-tools/hadoop-azure/src/main/resources/META-INF/services/org.apache.hadoop.security.token.TokenRenewer
          • (add) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/security/WasbTokenRenewer.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11430 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11430/ ) HADOOP-13945 . Azure: Add Kerberos and Delegation token support to WASB (liuml07: rev 8e15e240597f821968e14893eabfea39815de207) (edit) hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/MockWasbAuthorizerImpl.java (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/RemoteSASKeyGeneratorImpl.java (add) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/security/WasbDelegationTokenIdentifier.java (add) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/security/package-info.java (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/WasbAuthorizerInterface.java (add) hadoop-tools/hadoop-azure/src/main/resources/META-INF/services/org.apache.hadoop.security.token.TokenIdentifier (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/RemoteWasbAuthorizerImpl.java (add) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/security/SecurityUtils.java (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/SecureStorageInterfaceImpl.java (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java (add) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/security/Constants.java (add) hadoop-tools/hadoop-azure/src/main/resources/META-INF/services/org.apache.hadoop.security.token.TokenRenewer (add) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/security/WasbTokenRenewer.java
          Hide
          liuml07 Mingliang Liu added a comment -

          +1 on v11 patch.

          Let's get this in first and make it better in follow-up JIRAs. I've committed to branch-2 and trunk branches. Thanks for your contribution Santhosh G Nayak. Thanks for your review Steve Loughran.

          Show
          liuml07 Mingliang Liu added a comment - +1 on v11 patch. Let's get this in first and make it better in follow-up JIRAs. I've committed to branch-2 and trunk branches. Thanks for your contribution Santhosh G Nayak . Thanks for your review Steve Loughran .
          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 1 new or modified test files.
          +1 mvninstall 12m 17s trunk passed
          +1 compile 0m 17s trunk passed
          +1 checkstyle 0m 15s trunk passed
          +1 mvnsite 0m 19s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 0m 25s trunk passed
          +1 javadoc 0m 13s trunk passed
          +1 mvninstall 0m 16s the patch passed
          +1 compile 0m 15s the patch passed
          +1 javac 0m 15s the patch passed
          -0 checkstyle 0m 12s hadoop-tools/hadoop-azure: The patch generated 1 new + 74 unchanged - 2 fixed = 75 total (was 76)
          +1 mvnsite 0m 16s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 31s the patch passed
          +1 javadoc 0m 10s the patch passed
          +1 unit 1m 23s hadoop-azure in the patch passed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          19m 6s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859497/HADOOP-13945.11.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux f7cc0ac32745 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 34a931c
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11854/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11854/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11854/console
          Powered by Apache Yetus 0.5.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 1 new or modified test files. +1 mvninstall 12m 17s trunk passed +1 compile 0m 17s trunk passed +1 checkstyle 0m 15s trunk passed +1 mvnsite 0m 19s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 25s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 0m 15s the patch passed +1 javac 0m 15s the patch passed -0 checkstyle 0m 12s hadoop-tools/hadoop-azure: The patch generated 1 new + 74 unchanged - 2 fixed = 75 total (was 76) +1 mvnsite 0m 16s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 31s the patch passed +1 javadoc 0m 10s the patch passed +1 unit 1m 23s hadoop-azure in the patch passed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 19m 6s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859497/HADOOP-13945.11.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux f7cc0ac32745 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 34a931c Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11854/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11854/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11854/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          snayak Santhosh G Nayak added a comment -

          Thanks Mingliang Liu. Patch #10 looks good. I have added similar exception handling to RemoteWasbAuthorizerImpl.init() in patch #11. Steve Loughran Could you please review?

          Show
          snayak Santhosh G Nayak added a comment - Thanks Mingliang Liu . Patch #10 looks good. I have added similar exception handling to RemoteWasbAuthorizerImpl.init() in patch #11. Steve Loughran Could you please review?
          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 1 new or modified test files.
          +1 mvninstall 13m 48s trunk passed
          +1 compile 0m 18s trunk passed
          +1 checkstyle 0m 14s trunk passed
          +1 mvnsite 0m 23s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 0m 31s trunk passed
          +1 javadoc 0m 15s trunk passed
          +1 mvninstall 0m 17s the patch passed
          +1 compile 0m 15s the patch passed
          +1 javac 0m 15s the patch passed
          -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 1 new + 74 unchanged - 2 fixed = 75 total (was 76)
          +1 mvnsite 0m 20s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 37s the patch passed
          +1 javadoc 0m 13s the patch passed
          +1 unit 1m 32s hadoop-azure in the patch passed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          21m 26s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859385/HADOOP-13945.10.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 22055b1cb96a 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / e1a9980
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11848/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11848/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11848/console
          Powered by Apache Yetus 0.5.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 1 new or modified test files. +1 mvninstall 13m 48s trunk passed +1 compile 0m 18s trunk passed +1 checkstyle 0m 14s trunk passed +1 mvnsite 0m 23s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 31s trunk passed +1 javadoc 0m 15s trunk passed +1 mvninstall 0m 17s the patch passed +1 compile 0m 15s the patch passed +1 javac 0m 15s the patch passed -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 1 new + 74 unchanged - 2 fixed = 75 total (was 76) +1 mvnsite 0m 20s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 37s the patch passed +1 javadoc 0m 13s the patch passed +1 unit 1m 32s hadoop-azure in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 21m 26s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859385/HADOOP-13945.10.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 22055b1cb96a 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e1a9980 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11848/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11848/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11848/console Powered by Apache Yetus 0.5.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 1 new or modified test files.
          +1 mvninstall 12m 43s trunk passed
          +1 compile 0m 22s trunk passed
          +1 checkstyle 0m 16s trunk passed
          +1 mvnsite 0m 23s trunk passed
          +1 mvneclipse 0m 15s trunk passed
          +1 findbugs 0m 33s trunk passed
          +1 javadoc 0m 14s trunk passed
          +1 mvninstall 0m 21s the patch passed
          +1 compile 0m 20s the patch passed
          +1 javac 0m 20s the patch passed
          -0 checkstyle 0m 13s hadoop-tools/hadoop-azure: The patch generated 4 new + 74 unchanged - 2 fixed = 78 total (was 76)
          +1 mvnsite 0m 19s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 36s the patch passed
          +1 javadoc 0m 11s the patch passed
          +1 unit 1m 31s hadoop-azure in the patch passed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          20m 20s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859380/HADOOP-13945.9.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c9d88b98dbf6 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 7f8e928
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11846/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11846/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11846/console
          Powered by Apache Yetus 0.5.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 12m 43s trunk passed +1 compile 0m 22s trunk passed +1 checkstyle 0m 16s trunk passed +1 mvnsite 0m 23s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 0m 33s trunk passed +1 javadoc 0m 14s trunk passed +1 mvninstall 0m 21s the patch passed +1 compile 0m 20s the patch passed +1 javac 0m 20s the patch passed -0 checkstyle 0m 13s hadoop-tools/hadoop-azure: The patch generated 4 new + 74 unchanged - 2 fixed = 78 total (was 76) +1 mvnsite 0m 19s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 36s the patch passed +1 javadoc 0m 11s the patch passed +1 unit 1m 31s hadoop-azure in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 20m 20s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859380/HADOOP-13945.9.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c9d88b98dbf6 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 7f8e928 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11846/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11846/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11846/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          liuml07 Mingliang Liu added a comment -

          I refined the v8 patch according to Steve's comments. Please confirm, Santhosh G Nayak. Can you give it another review Steve Loughran and Jitendra Nath Pandey?

          Show
          liuml07 Mingliang Liu added a comment - I refined the v8 patch according to Steve's comments. Please confirm, Santhosh G Nayak . Can you give it another review Steve Loughran and Jitendra Nath Pandey ?
          Hide
          stevel@apache.org Steve Loughran added a comment -

          in RemoteWasbAuthorizerImpl.getSASKey() ,InterruptedException should be rethrow, or the thread interrupte state set...don't want to lose the shutdown event.
          as any raised IOE text isn't preserved, the SASKeyGenerationException which be raised afterwards will lose the text/stack trace. Needs to be preserved for the sake of the support team.

          Similarly, RemoteWasbAuthorizerImpl.init() logs an IOE but doesn't retain it to rethrow. Should be used to init the cause in the wasb exception.

          Show
          stevel@apache.org Steve Loughran added a comment - in RemoteWasbAuthorizerImpl.getSASKey() ,InterruptedException should be rethrow, or the thread interrupte state set...don't want to lose the shutdown event. as any raised IOE text isn't preserved, the SASKeyGenerationException which be raised afterwards will lose the text/stack trace. Needs to be preserved for the sake of the support team. Similarly, RemoteWasbAuthorizerImpl.init() logs an IOE but doesn't retain it to rethrow. Should be used to init the cause in the wasb exception.
          Hide
          snayak Santhosh G Nayak added a comment -

          Mingliang Liu, Fixed the javadoc and checkstyle warnings in patch #8.

          Show
          snayak Santhosh G Nayak added a comment - Mingliang Liu , Fixed the javadoc and checkstyle warnings in patch #8.
          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 12m 13s trunk passed
          +1 compile 0m 17s trunk passed
          +1 checkstyle 0m 14s trunk passed
          +1 mvnsite 0m 19s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 0m 26s trunk passed
          +1 javadoc 0m 13s trunk passed
          +1 mvninstall 0m 16s the patch passed
          +1 compile 0m 14s the patch passed
          +1 javac 0m 14s the patch passed
          -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 1 new + 74 unchanged - 2 fixed = 75 total (was 76)
          +1 mvnsite 0m 17s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 31s the patch passed
          +1 javadoc 0m 10s the patch passed
          +1 unit 1m 23s hadoop-azure in the patch passed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          19m 5s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859043/HADOOP-13945.8.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux a5e4f7d3ac14 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 6d95866
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11834/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11834/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11834/console
          Powered by Apache Yetus 0.5.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 12m 13s trunk passed +1 compile 0m 17s trunk passed +1 checkstyle 0m 14s trunk passed +1 mvnsite 0m 19s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 26s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 0m 14s the patch passed +1 javac 0m 14s the patch passed -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 1 new + 74 unchanged - 2 fixed = 75 total (was 76) +1 mvnsite 0m 17s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 31s the patch passed +1 javadoc 0m 10s the patch passed +1 unit 1m 23s hadoop-azure in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 19m 5s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859043/HADOOP-13945.8.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a5e4f7d3ac14 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6d95866 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11834/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11834/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11834/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          liuml07 Mingliang Liu added a comment -

          Can you fix the related javadoc and checkstyle warnings?

          I'll commit this in 2 days if no further comment received.

          Show
          liuml07 Mingliang Liu added a comment - Can you fix the related javadoc and checkstyle warnings? I'll commit this in 2 days if no further comment received.
          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 1 new or modified test files.
          +1 mvninstall 12m 32s trunk passed
          +1 compile 0m 18s trunk passed
          +1 checkstyle 0m 15s trunk passed
          +1 mvnsite 0m 19s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 26s trunk passed
          +1 javadoc 0m 13s trunk passed
          +1 mvninstall 0m 16s the patch passed
          +1 compile 0m 15s the patch passed
          +1 javac 0m 15s the patch passed
          -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 7 new + 74 unchanged - 2 fixed = 81 total (was 76)
          +1 mvnsite 0m 16s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 30s the patch passed
          -1 javadoc 0m 10s hadoop-tools_hadoop-azure generated 16 new + 0 unchanged - 0 fixed = 16 total (was 0)
          +1 unit 1m 23s hadoop-azure in the patch passed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          19m 27s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858934/HADOOP-13945.7.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux cb75a090d9d0 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 615ac09
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11830/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11830/artifact/patchprocess/diff-javadoc-javadoc-hadoop-tools_hadoop-azure.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11830/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11830/console
          Powered by Apache Yetus 0.5.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 1 new or modified test files. +1 mvninstall 12m 32s trunk passed +1 compile 0m 18s trunk passed +1 checkstyle 0m 15s trunk passed +1 mvnsite 0m 19s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 26s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 0m 15s the patch passed +1 javac 0m 15s the patch passed -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 7 new + 74 unchanged - 2 fixed = 81 total (was 76) +1 mvnsite 0m 16s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 30s the patch passed -1 javadoc 0m 10s hadoop-tools_hadoop-azure generated 16 new + 0 unchanged - 0 fixed = 16 total (was 0) +1 unit 1m 23s hadoop-azure in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 19m 27s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858934/HADOOP-13945.7.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux cb75a090d9d0 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 615ac09 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11830/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11830/artifact/patchprocess/diff-javadoc-javadoc-hadoop-tools_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11830/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11830/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          snayak Santhosh G Nayak added a comment -

          Mingliang Liu, Thanks for reviewing the patch.
          I have addressed the review comments in patch #7.
          (1) Added StringUtils.isNotEmpty(delegationToken) instead of explicit null and empty checks.
          (2) Wrapped InterruptedException inside WasbAuthorizationException, I wanted to throw only meaningful exceptions to the above layer.
          (3) Added javadoc for newly added methods.
          (4) I have tested the patch in real secure Hadoop cluster with WASB storage by running the basic fs operation shell commands, MR jobs and hive jobs.

          Show
          snayak Santhosh G Nayak added a comment - Mingliang Liu , Thanks for reviewing the patch. I have addressed the review comments in patch #7. (1) Added StringUtils.isNotEmpty(delegationToken) instead of explicit null and empty checks. (2) Wrapped InterruptedException inside WasbAuthorizationException , I wanted to throw only meaningful exceptions to the above layer. (3) Added javadoc for newly added methods. (4) I have tested the patch in real secure Hadoop cluster with WASB storage by running the basic fs operation shell commands, MR jobs and hive jobs.
          Hide
          liuml07 Mingliang Liu added a comment -

          Patch looks good overall, again. Comments:

          1. Following can be StringUtils.isNotEmpty(delegationToken) for last two checks in if condition. I saw three places like this along with StringUtils.isEmpty() for null&empty check.
            	      if (isSecurityEnabled && delegationToken != null && !delegationToken
            	          .isEmpty()) {
            
          2. In RemoteWasbAuthorizerImpl:: authorize(), should we throw InterruptedIOException when the authorize gets Interrupted exception?
            RemoteWasbAuthorizerImpl:: authorize()
            158	        try {
            159	          responseBody = connectUgi
            160	              .doAs(new PrivilegedExceptionAction<String>() {
            161	                @Override
            162	                public String run() throws Exception {
            ...                     ...
            185	              });
            186	        } catch (InterruptedException e) {
            187	          LOG.error("Error in check authorization", e);
            188	        }
            
          3. Can you add javadoc for newly added methods?
          4. Can you confirm that the integration tests pass?

          If Steve Loughran can provide a 2nd opinion, it will be great.

          Show
          liuml07 Mingliang Liu added a comment - Patch looks good overall, again. Comments: Following can be StringUtils.isNotEmpty(delegationToken) for last two checks in if condition. I saw three places like this along with StringUtils.isEmpty() for null&empty check. if (isSecurityEnabled && delegationToken != null && !delegationToken .isEmpty()) { In RemoteWasbAuthorizerImpl:: authorize() , should we throw InterruptedIOException when the authorize gets Interrupted exception? RemoteWasbAuthorizerImpl:: authorize() 158 try { 159 responseBody = connectUgi 160 .doAs( new PrivilegedExceptionAction< String >() { 161 @Override 162 public String run() throws Exception { ... ... 185 }); 186 } catch (InterruptedException e) { 187 LOG.error( "Error in check authorization" , e); 188 } Can you add javadoc for newly added methods? Can you confirm that the integration tests pass? If Steve Loughran can provide a 2nd opinion, it will be great.
          Hide
          liuml07 Mingliang Liu added a comment -

          Thanks for updating the patch! I'll review this week.

          Show
          liuml07 Mingliang Liu added a comment - Thanks for updating the patch! I'll review this week.
          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 12m 26s trunk passed
          +1 compile 0m 18s trunk passed
          +1 checkstyle 0m 14s trunk passed
          +1 mvnsite 0m 19s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 0m 25s trunk passed
          +1 javadoc 0m 13s trunk passed
          +1 mvninstall 0m 16s the patch passed
          +1 compile 0m 14s the patch passed
          +1 javac 0m 14s the patch passed
          -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 7 new + 74 unchanged - 2 fixed = 81 total (was 76)
          +1 mvnsite 0m 17s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 31s the patch passed
          +1 javadoc 0m 11s the patch passed
          +1 unit 1m 23s hadoop-azure in the patch passed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          19m 12s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12857002/HADOOP-13945.6.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 69f0bb7cc1d2 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 385d2cb
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11788/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11788/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11788/console
          Powered by Apache Yetus 0.5.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 12m 26s trunk passed +1 compile 0m 18s trunk passed +1 checkstyle 0m 14s trunk passed +1 mvnsite 0m 19s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 25s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 0m 14s the patch passed +1 javac 0m 14s the patch passed -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 7 new + 74 unchanged - 2 fixed = 81 total (was 76) +1 mvnsite 0m 17s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 31s the patch passed +1 javadoc 0m 11s the patch passed +1 unit 1m 23s hadoop-azure in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 19m 12s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12857002/HADOOP-13945.6.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 69f0bb7cc1d2 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 385d2cb Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11788/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11788/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11788/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          snayak Santhosh G Nayak added a comment -

          Thanks Mingliang Liu for reviewing the patch.

          I have created a rebased patch excluding the changes in HADOOP-13930 (as it is already committed) and addressed the following review comments,

          (1) fs.azure.authorization.remote.service.url was introduced in HADOOP-13930. Current patch does not have any reference to it.

          (2) In code to log a message, keeping the exception as well.

          (3) Unfortunately, UserGroupInformation.getCurrentUser().getCredentials().getToken(WasbDelegationTokenIdentifier.TOKEN_KIND) does not work, it takes only alias. Moving this logic to a util method.

          (4) Fixed the nit by removing () for && in if (isSecurityEnabled && (delegationToken != null && !delegationToken.isEmpty())).

          (5) Added package-info.java instead of package.html.

          (6) Created Util methods to avoid duplicate code wherever possible.

          (7), (8), (9) and (10) comments are related changes in HADOOP-13930 and already addressed there.

          (11) Handled it appropriately to the best of my knowledge, let me know if think otherwise.

          Show
          snayak Santhosh G Nayak added a comment - Thanks Mingliang Liu for reviewing the patch. I have created a rebased patch excluding the changes in HADOOP-13930 (as it is already committed) and addressed the following review comments, (1) fs.azure.authorization.remote.service.url was introduced in HADOOP-13930 . Current patch does not have any reference to it. (2) In code to log a message, keeping the exception as well. (3) Unfortunately, UserGroupInformation.getCurrentUser().getCredentials().getToken(WasbDelegationTokenIdentifier.TOKEN_KIND) does not work, it takes only alias. Moving this logic to a util method. (4) Fixed the nit by removing () for && in if (isSecurityEnabled && (delegationToken != null && !delegationToken.isEmpty())) . (5) Added package-info.java instead of package.html . (6) Created Util methods to avoid duplicate code wherever possible. (7), (8), (9) and (10) comments are related changes in HADOOP-13930 and already addressed there. (11) Handled it appropriately to the best of my knowledge, let me know if think otherwise.
          Hide
          liuml07 Mingliang Liu added a comment -

          As there are a lot of shared code to HADOOP-13930, I'll suggest we rebase the patch here after that one is committed.

          Show
          liuml07 Mingliang Liu added a comment - As there are a lot of shared code to HADOOP-13930 , I'll suggest we rebase the patch here after that one is committed.
          Hide
          liuml07 Mingliang Liu added a comment - - edited

          Patch looks good to me overall. For a few places using UGI, I need 2nd opinion. Steve Loughran I'll hold on commit 3 days in case you'd like to review.

          1. fs.azure.authorization.remote.service.url should be a separate constant final variable as well as a config key (e.g. in core-default.xml)?
          2. In code to log a message, let's keep the exception in log message. e.g.
            RemoteSASKeyGeneratorImpl#initialize()
            LOG.error("Error in fetching the WASB delegation token");
            
          3. I see at least two places using the logic of finding the expected token,
            114	    Iterator<Token<? extends TokenIdentifier>> tokenIterator = null;
            115	    try {
            116	      tokenIterator = UserGroupInformation.getCurrentUser().getCredentials()
            117	          .getAllTokens().iterator();
            118	      while (tokenIterator.hasNext()) {
            119	        Token<? extends TokenIdentifier> iteratedToken = tokenIterator.next();
            120	        if (iteratedToken.getKind().equals(WasbDelegationTokenIdentifier.TOKEN_KIND)) {
            121	          delegationToken = iteratedToken.encodeToUrlString();
            122	        }
            123	      }
            

            Can we use UserGroupInformation.getCurrentUser().getCredentials().getToken(WasbDelegationTokenIdentifier.TOKEN_KIND)? We have to test this.

          4. if (isSecurityEnabled && (delegationToken != null && !delegationToken.isEmpty())). This is a nit. We don't need () for && right?
          5. package-info.java is preferred to package.html since Java 5.
          6. I see a few duplicate code, can we create a helper method for that? e.g.
            94	    final UserGroupInformation ugi = UserGroupInformation.getCurrentUser();
            95	    UserGroupInformation connectUgi = ugi.getRealUser();
            96	    final UserGroupInformation proxyUser = connectUgi;
            97	    if (connectUgi == null) {
            98	      connectUgi = ugi;
            99	    }
            100	    if(!connectUgi.hasKerberosCredentials()){
            101	      connectUgi = UserGroupInformation.getLoginUser();
            102	    }
            

            and

            72	    final String credServiceUrl = conf.get(Constants.KEY_CRED_SERVICE_URL,
            73	        String.format("http://%s:%s",
            74	            InetAddress.getLocalHost().getCanonicalHostName(),
            75	            Constants.DEFAULT_CRED_SERVICE_PORT));
            
          7. For unit tests, we should add more assertion with clear error message if the test fails.
          8. ContractTestUtils has a few helper methods that can be used. Please consider that.
          9. For unit tests, we need to use paths which are unique for the test suite; using the working path doesn't do that. We need this for parallel test execution. See Steve's comment in HADOOP-13930.
          10. test path must be robustly deleted in teardown, e.g. write a @After tearDown() method to clean up the parent directory. This method will be executed if the test fails with exception.
          11. try-catch and rethrow exception is not ideal. Fail with clearer error message will be better.
          Show
          liuml07 Mingliang Liu added a comment - - edited Patch looks good to me overall. For a few places using UGI, I need 2nd opinion. Steve Loughran I'll hold on commit 3 days in case you'd like to review. fs.azure.authorization.remote.service.url should be a separate constant final variable as well as a config key (e.g. in core-default.xml)? In code to log a message, let's keep the exception in log message. e.g. RemoteSASKeyGeneratorImpl#initialize() LOG.error( "Error in fetching the WASB delegation token" ); I see at least two places using the logic of finding the expected token, 114 Iterator<Token<? extends TokenIdentifier>> tokenIterator = null ; 115 try { 116 tokenIterator = UserGroupInformation.getCurrentUser().getCredentials() 117 .getAllTokens().iterator(); 118 while (tokenIterator.hasNext()) { 119 Token<? extends TokenIdentifier> iteratedToken = tokenIterator.next(); 120 if (iteratedToken.getKind().equals(WasbDelegationTokenIdentifier.TOKEN_KIND)) { 121 delegationToken = iteratedToken.encodeToUrlString(); 122 } 123 } Can we use UserGroupInformation.getCurrentUser().getCredentials().getToken(WasbDelegationTokenIdentifier.TOKEN_KIND) ? We have to test this. if (isSecurityEnabled && (delegationToken != null && !delegationToken.isEmpty())) . This is a nit. We don't need () for && right? package-info.java is preferred to package.html since Java 5. I see a few duplicate code, can we create a helper method for that? e.g. 94 final UserGroupInformation ugi = UserGroupInformation.getCurrentUser(); 95 UserGroupInformation connectUgi = ugi.getRealUser(); 96 final UserGroupInformation proxyUser = connectUgi; 97 if (connectUgi == null ) { 98 connectUgi = ugi; 99 } 100 if (!connectUgi.hasKerberosCredentials()){ 101 connectUgi = UserGroupInformation.getLoginUser(); 102 } and 72 final String credServiceUrl = conf.get(Constants.KEY_CRED_SERVICE_URL, 73 String .format( "http: //%s:%s" , 74 InetAddress.getLocalHost().getCanonicalHostName(), 75 Constants.DEFAULT_CRED_SERVICE_PORT)); For unit tests, we should add more assertion with clear error message if the test fails. ContractTestUtils has a few helper methods that can be used. Please consider that. For unit tests, we need to use paths which are unique for the test suite; using the working path doesn't do that. We need this for parallel test execution. See Steve's comment in HADOOP-13930 . test path must be robustly deleted in teardown, e.g. write a @After tearDown() method to clean up the parent directory. This method will be executed if the test fails with exception. try-catch and rethrow exception is not ideal. Fail with clearer error message will be better.
          Hide
          liuml07 Mingliang Liu added a comment -

          YARN UI module is definitely not related so I'll re-trigger the Jenkins run late today.

          I'll post the review soon; the v5 patch overall looks good to me.

          Show
          liuml07 Mingliang Liu added a comment - YARN UI module is definitely not related so I'll re-trigger the Jenkins run late today. I'll post the review soon; the v5 patch overall looks good to me.
          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 3 new or modified test files.
          0 mvndep 1m 54s Maven dependency ordering for branch
          +1 mvninstall 12m 36s trunk passed
          -1 compile 8m 52s root in trunk failed.
          +1 checkstyle 1m 55s trunk passed
          +1 mvnsite 1m 27s trunk passed
          +1 mvneclipse 0m 38s trunk passed
          +1 findbugs 2m 1s trunk passed
          +1 javadoc 1m 11s trunk passed
          0 mvndep 0m 17s Maven dependency ordering for patch
          +1 mvninstall 0m 57s the patch passed
          -1 compile 7m 40s root in the patch failed.
          -1 javac 7m 40s root in the patch failed.
          -0 checkstyle 1m 54s root: The patch generated 19 new + 71 unchanged - 0 fixed = 90 total (was 71)
          +1 mvnsite 1m 27s the patch passed
          +1 mvneclipse 0m 42s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 2m 22s the patch passed
          -1 javadoc 0m 24s hadoop-tools_hadoop-azure generated 4 new + 1 unchanged - 0 fixed = 5 total (was 1)
          -1 unit 8m 14s hadoop-common in the patch failed.
          +1 unit 1m 35s hadoop-azure in the patch passed.
          +1 asflicense 0m 37s The patch does not generate ASF License warnings.
          82m 27s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855838/HADOOP-13945.5.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux 4f2ea358b16f 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / eb5a179
          Default Java 1.8.0_121
          compile https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/branch-compile-root.txt
          findbugs v3.0.0
          compile https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/patch-compile-root.txt
          javac https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/patch-compile-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/diff-checkstyle-root.txt
          javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/diff-javadoc-javadoc-hadoop-tools_hadoop-azure.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/console
          Powered by Apache Yetus 0.5.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 3 new or modified test files. 0 mvndep 1m 54s Maven dependency ordering for branch +1 mvninstall 12m 36s trunk passed -1 compile 8m 52s root in trunk failed. +1 checkstyle 1m 55s trunk passed +1 mvnsite 1m 27s trunk passed +1 mvneclipse 0m 38s trunk passed +1 findbugs 2m 1s trunk passed +1 javadoc 1m 11s trunk passed 0 mvndep 0m 17s Maven dependency ordering for patch +1 mvninstall 0m 57s the patch passed -1 compile 7m 40s root in the patch failed. -1 javac 7m 40s root in the patch failed. -0 checkstyle 1m 54s root: The patch generated 19 new + 71 unchanged - 0 fixed = 90 total (was 71) +1 mvnsite 1m 27s the patch passed +1 mvneclipse 0m 42s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 2m 22s the patch passed -1 javadoc 0m 24s hadoop-tools_hadoop-azure generated 4 new + 1 unchanged - 0 fixed = 5 total (was 1) -1 unit 8m 14s hadoop-common in the patch failed. +1 unit 1m 35s hadoop-azure in the patch passed. +1 asflicense 0m 37s The patch does not generate ASF License warnings. 82m 27s Reason Tests Failed junit tests hadoop.security.TestKDiag Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855838/HADOOP-13945.5.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux 4f2ea358b16f 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / eb5a179 Default Java 1.8.0_121 compile https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/branch-compile-root.txt findbugs v3.0.0 compile https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/patch-compile-root.txt javac https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/patch-compile-root.txt checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/diff-checkstyle-root.txt javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/diff-javadoc-javadoc-hadoop-tools_hadoop-azure.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11757/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          snayak Santhosh G Nayak added a comment -

          Mingliang Liu, Thanks for reviewing the patch.

          In the latest patch, I have incorporated following,

          • @return annotation in the init() is removed as it is void return type.
          • Marked DEFAULT_AZURE_AUTHORIZATION as final.
          • Bug fix - Using UserGroupInformation#getLoginUser() whenever kerberos credentials are not available for UserGroupInformation#getCurrentUser().
          Show
          snayak Santhosh G Nayak added a comment - Mingliang Liu , Thanks for reviewing the patch. In the latest patch, I have incorporated following, @return annotation in the init() is removed as it is void return type. Marked DEFAULT_AZURE_AUTHORIZATION as final. Bug fix - Using UserGroupInformation#getLoginUser() whenever kerberos credentials are not available for UserGroupInformation#getCurrentUser() .
          Hide
          liuml07 Mingliang Liu added a comment -

          Thanks for updating the patch. I'll review the v4 patch this week.

          Early comments:

          35	   * @return True - If initialization successful
          36	   *         False - Otherwise
          37	   */
          38	  public void init(Configuration conf)
          39	      throws WasbAuthorizationException, IOException;
          

          The @return is wrongly annotated as the init() is void return type.

          Failing unit test is not related.
          We can checkstyle warning before commit, e,g, to make DEFAULT_AZURE_AUTHORIZATION final. Specially file length warning can be ignored as it's mostly existing and can be fixed separately.

          Show
          liuml07 Mingliang Liu added a comment - Thanks for updating the patch. I'll review the v4 patch this week. Early comments: 35 * @ return True - If initialization successful 36 * False - Otherwise 37 */ 38 public void init(Configuration conf) 39 throws WasbAuthorizationException, IOException; The @return is wrongly annotated as the init() is void return type. Failing unit test is not related. We can checkstyle warning before commit, e,g, to make DEFAULT_AZURE_AUTHORIZATION final. Specially file length warning can be ignored as it's mostly existing and can be fixed separately.
          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 3 new or modified test files.
          0 mvndep 0m 14s Maven dependency ordering for branch
          +1 mvninstall 13m 25s trunk passed
          +1 compile 13m 36s trunk passed
          +1 checkstyle 1m 30s trunk passed
          +1 mvnsite 1m 26s trunk passed
          +1 mvneclipse 0m 37s trunk passed
          +1 findbugs 2m 0s trunk passed
          +1 javadoc 1m 10s trunk passed
          0 mvndep 0m 16s Maven dependency ordering for patch
          +1 mvninstall 0m 58s the patch passed
          +1 compile 11m 13s the patch passed
          +1 javac 11m 13s the patch passed
          -0 checkstyle 1m 34s root: The patch generated 6 new + 48 unchanged - 0 fixed = 54 total (was 48)
          +1 mvnsite 1m 30s the patch passed
          +1 mvneclipse 0m 45s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 2m 21s the patch passed
          -1 javadoc 0m 24s hadoop-azure in the patch failed.
          -1 unit 8m 14s hadoop-common in the patch failed.
          +1 unit 1m 41s hadoop-azure in the patch passed.
          +1 asflicense 0m 38s The patch does not generate ASF License warnings.
          89m 23s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12852523/HADOOP-13945.4.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux ac02e5dd1129 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 71c23c9
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11622/artifact/patchprocess/diff-checkstyle-root.txt
          javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11622/artifact/patchprocess/patch-javadoc-hadoop-tools_hadoop-azure.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11622/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11622/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11622/console
          Powered by Apache Yetus 0.5.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 3 new or modified test files. 0 mvndep 0m 14s Maven dependency ordering for branch +1 mvninstall 13m 25s trunk passed +1 compile 13m 36s trunk passed +1 checkstyle 1m 30s trunk passed +1 mvnsite 1m 26s trunk passed +1 mvneclipse 0m 37s trunk passed +1 findbugs 2m 0s trunk passed +1 javadoc 1m 10s trunk passed 0 mvndep 0m 16s Maven dependency ordering for patch +1 mvninstall 0m 58s the patch passed +1 compile 11m 13s the patch passed +1 javac 11m 13s the patch passed -0 checkstyle 1m 34s root: The patch generated 6 new + 48 unchanged - 0 fixed = 54 total (was 48) +1 mvnsite 1m 30s the patch passed +1 mvneclipse 0m 45s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 2m 21s the patch passed -1 javadoc 0m 24s hadoop-azure in the patch failed. -1 unit 8m 14s hadoop-common in the patch failed. +1 unit 1m 41s hadoop-azure in the patch passed. +1 asflicense 0m 38s The patch does not generate ASF License warnings. 89m 23s Reason Tests Failed junit tests hadoop.security.TestKDiag Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12852523/HADOOP-13945.4.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux ac02e5dd1129 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 71c23c9 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11622/artifact/patchprocess/diff-checkstyle-root.txt javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11622/artifact/patchprocess/patch-javadoc-hadoop-tools_hadoop-azure.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11622/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11622/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11622/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          snayak Santhosh G Nayak added a comment -

          Thanks Mingliang Liu, I have fixed the unit test hadoop.conf.TestCommonConfigurationFields failure in the new patch.

          Show
          snayak Santhosh G Nayak added a comment - Thanks Mingliang Liu , I have fixed the unit test hadoop.conf.TestCommonConfigurationFields failure in the new patch.
          Hide
          liuml07 Mingliang Liu added a comment -

          Can you provide a new patch to get a clean Jenkins pre-commit run? Specially, to fix the failing unit test hadoop.conf.TestCommonConfigurationFields, we can skip the newly added config in method initializeMemberVariables().

          Show
          liuml07 Mingliang Liu added a comment - Can you provide a new patch to get a clean Jenkins pre-commit run? Specially, to fix the failing unit test hadoop.conf.TestCommonConfigurationFields , we can skip the newly added config in method initializeMemberVariables() .
          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.
          0 mvndep 0m 16s Maven dependency ordering for branch
          +1 mvninstall 12m 56s trunk passed
          +1 compile 10m 25s trunk passed
          +1 checkstyle 1m 34s trunk passed
          +1 mvnsite 1m 30s trunk passed
          +1 mvneclipse 0m 37s trunk passed
          +1 findbugs 2m 3s trunk passed
          +1 javadoc 1m 9s trunk passed
          0 mvndep 0m 17s Maven dependency ordering for patch
          +1 mvninstall 0m 57s the patch passed
          +1 compile 10m 30s the patch passed
          +1 javac 10m 30s the patch passed
          -0 checkstyle 1m 35s root: The patch generated 8 new + 43 unchanged - 0 fixed = 51 total (was 43)
          +1 mvnsite 1m 36s the patch passed
          +1 mvneclipse 0m 42s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          -1 findbugs 0m 47s hadoop-tools/hadoop-azure generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          -1 javadoc 0m 24s hadoop-azure in the patch failed.
          -1 unit 8m 27s hadoop-common in the patch failed.
          +1 unit 1m 44s hadoop-azure in the patch passed.
          +1 asflicense 0m 35s The patch does not generate ASF License warnings.
          84m 55s



          Reason Tests
          FindBugs module:hadoop-tools/hadoop-azure
            org.apache.hadoop.fs.azure.NativeAzureFileSystem.KEY_AZURE_AUTHORIZATION isn't final but should be At NativeAzureFileSystem.java:be At NativeAzureFileSystem.java:[line 1114]
          Failed junit tests hadoop.conf.TestCommonConfigurationFields



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12847426/HADOOP-13945.3.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux 96e6552fc383 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / d3170f9
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/artifact/patchprocess/diff-checkstyle-root.txt
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/artifact/patchprocess/new-findbugs-hadoop-tools_hadoop-azure.html
          javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/artifact/patchprocess/patch-javadoc-hadoop-tools_hadoop-azure.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/console
          Powered by Apache Yetus 0.5.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. 0 mvndep 0m 16s Maven dependency ordering for branch +1 mvninstall 12m 56s trunk passed +1 compile 10m 25s trunk passed +1 checkstyle 1m 34s trunk passed +1 mvnsite 1m 30s trunk passed +1 mvneclipse 0m 37s trunk passed +1 findbugs 2m 3s trunk passed +1 javadoc 1m 9s trunk passed 0 mvndep 0m 17s Maven dependency ordering for patch +1 mvninstall 0m 57s the patch passed +1 compile 10m 30s the patch passed +1 javac 10m 30s the patch passed -0 checkstyle 1m 35s root: The patch generated 8 new + 43 unchanged - 0 fixed = 51 total (was 43) +1 mvnsite 1m 36s the patch passed +1 mvneclipse 0m 42s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. -1 findbugs 0m 47s hadoop-tools/hadoop-azure generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) -1 javadoc 0m 24s hadoop-azure in the patch failed. -1 unit 8m 27s hadoop-common in the patch failed. +1 unit 1m 44s hadoop-azure in the patch passed. +1 asflicense 0m 35s The patch does not generate ASF License warnings. 84m 55s Reason Tests FindBugs module:hadoop-tools/hadoop-azure   org.apache.hadoop.fs.azure.NativeAzureFileSystem.KEY_AZURE_AUTHORIZATION isn't final but should be At NativeAzureFileSystem.java:be At NativeAzureFileSystem.java: [line 1114] Failed junit tests hadoop.conf.TestCommonConfigurationFields Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12847426/HADOOP-13945.3.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux 96e6552fc383 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / d3170f9 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/artifact/patchprocess/diff-checkstyle-root.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/artifact/patchprocess/new-findbugs-hadoop-tools_hadoop-azure.html javadoc https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/artifact/patchprocess/patch-javadoc-hadoop-tools_hadoop-azure.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-azure U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11435/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          snayak Santhosh G Nayak added a comment -

          Adding a patch which is created on top of https://issues.apache.org/jira/secure/attachment/12845516/HADOOP-13930.002.patch,
          Patch contains following additional changes,

          • Kerberos support for RemoteWasbAuthorizerImpl#authorize() request to remote server.
          • Added a configuration property fs.azure.enable.kerberos.support to enable/disable Kerberos support.
          • Support for impersonation(doAs) of a user with remote service.
          Show
          snayak Santhosh G Nayak added a comment - Adding a patch which is created on top of https://issues.apache.org/jira/secure/attachment/12845516/HADOOP-13930.002.patch , Patch contains following additional changes, Kerberos support for RemoteWasbAuthorizerImpl#authorize() request to remote server. Added a configuration property fs.azure.enable.kerberos.support to enable/disable Kerberos support. Support for impersonation(doAs) of a user with remote service.
          Hide
          liuml07 Mingliang Liu added a comment -

          Thanks for updating the patch. Looking forward to the refine version.

          Show
          liuml07 Mingliang Liu added a comment - Thanks for updating the patch. Looking forward to the refine version.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 11s 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 13m 2s trunk passed
          +1 compile 0m 17s trunk passed
          +1 checkstyle 0m 13s trunk passed
          +1 mvnsite 0m 20s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 0m 28s trunk passed
          +1 javadoc 0m 13s trunk passed
          +1 mvninstall 0m 18s the patch passed
          +1 compile 0m 16s the patch passed
          +1 javac 0m 16s the patch passed
          -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 4 new + 43 unchanged - 0 fixed = 47 total (was 43)
          +1 mvnsite 0m 18s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 0m 34s hadoop-tools/hadoop-azure generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 0m 10s the patch passed
          +1 unit 1m 24s hadoop-azure in the patch passed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          19m 56s



          Reason Tests
          FindBugs module:hadoop-tools/hadoop-azure
            Dead store to realUser in org.apache.hadoop.fs.azure.NativeAzureFileSystem.initialize(URI, Configuration) At NativeAzureFileSystem.java:org.apache.hadoop.fs.azure.NativeAzureFileSystem.initialize(URI, Configuration) At NativeAzureFileSystem.java:[line 1246]



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845545/HADOOP-13945.2.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux d5322f118152 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / e49e0a6
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11355/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/11355/artifact/patchprocess/new-findbugs-hadoop-tools_hadoop-azure.html
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11355/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11355/console
          Powered by Apache Yetus 0.5.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 11s 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 13m 2s trunk passed +1 compile 0m 17s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 20s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 28s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 18s the patch passed +1 compile 0m 16s the patch passed +1 javac 0m 16s the patch passed -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 4 new + 43 unchanged - 0 fixed = 47 total (was 43) +1 mvnsite 0m 18s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 0m 34s hadoop-tools/hadoop-azure generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 0m 10s the patch passed +1 unit 1m 24s hadoop-azure in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 19m 56s Reason Tests FindBugs module:hadoop-tools/hadoop-azure   Dead store to realUser in org.apache.hadoop.fs.azure.NativeAzureFileSystem.initialize(URI, Configuration) At NativeAzureFileSystem.java:org.apache.hadoop.fs.azure.NativeAzureFileSystem.initialize(URI, Configuration) At NativeAzureFileSystem.java: [line 1246] Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845545/HADOOP-13945.2.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux d5322f118152 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e49e0a6 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11355/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/11355/artifact/patchprocess/new-findbugs-hadoop-tools_hadoop-azure.html Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11355/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11355/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          snayak Santhosh G Nayak added a comment - - edited

          Mingliang Liu, Thanks for reviewing the patch. I have attached another working patch addressing following review comments,
          1) Added log statements with meaningful messages.
          2) I will try to add tests it in the next iteration.
          3) Marked Constants class as final with a private constructor.
          4) Not added getSASKey() method as static in the current iteration, as the method depends on the instance delegation token.
          5) Fixed most of checkstyle related warnings.
          6) I have not incorporated this change in the current iteration, as AbstractDelegationTokenSelector#selectToken() selects the token based on service and token kind. We wanted only based on the kind of the token.

          There are some more changes planned as part of this JIRA in subsequent iterations,

          • Adding retries and client side fail over mechanisms while trying to obtain delegation tokens and Azure Storage SAS keys from remote service.
          • Ability to provide customized connection configurator while trying to connect to remote service.
          Show
          snayak Santhosh G Nayak added a comment - - edited Mingliang Liu , Thanks for reviewing the patch. I have attached another working patch addressing following review comments, 1) Added log statements with meaningful messages. 2) I will try to add tests it in the next iteration. 3) Marked Constants class as final with a private constructor. 4) Not added getSASKey() method as static in the current iteration, as the method depends on the instance delegation token. 5) Fixed most of checkstyle related warnings. 6) I have not incorporated this change in the current iteration, as AbstractDelegationTokenSelector#selectToken() selects the token based on service and token kind. We wanted only based on the kind of the token. There are some more changes planned as part of this JIRA in subsequent iterations, Adding retries and client side fail over mechanisms while trying to obtain delegation tokens and Azure Storage SAS keys from remote service. Ability to provide customized connection configurator while trying to connect to remote service.
          Hide
          liuml07 Mingliang Liu added a comment -

          Very early comments are about 1) the logging messages: we should log more meaningful messages especially in case of exceptions; 2) can we add unit tests for this? 3) We can make Constants class final, along with a private constructor 4) getSASKey() static? 5) I'm expecting some checkstyle warnings (e.g. longer than 80 chars) 6) for selecting the tokens in RemoteSASKeyGeneratorImpl#initialize(), can we borrow the code in AbstractDelegationTokenSelector? I'm not sure about this.

          Show
          liuml07 Mingliang Liu added a comment - Very early comments are about 1) the logging messages: we should log more meaningful messages especially in case of exceptions; 2) can we add unit tests for this? 3) We can make Constants class final, along with a private constructor 4) getSASKey() static? 5) I'm expecting some checkstyle warnings (e.g. longer than 80 chars) 6) for selecting the tokens in RemoteSASKeyGeneratorImpl#initialize() , can we borrow the code in AbstractDelegationTokenSelector ? I'm not sure about this.
          Hide
          liuml07 Mingliang Liu added a comment -

          The patch does not apply. I used patch command and it failed as well. Can you have a look at this? The alternative approach is to use git format-patch trunk for generating patches in your working directory.

          Show
          liuml07 Mingliang Liu added a comment - The patch does not apply. I used patch command and it failed as well. Can you have a look at this? The alternative approach is to use git format-patch trunk for generating patches in your working directory.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



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



          Subsystem Report/Notes
          JIRA Issue HADOOP-13945
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845373/HADOOP-13945.1.patch
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11345/console
          Powered by Apache Yetus 0.5.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 4s HADOOP-13945 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Issue HADOOP-13945 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845373/HADOOP-13945.1.patch Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11345/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          liuml07 Mingliang Liu added a comment -

          Santhosh G Nayak, I added you to the Hadoop Common contributor list and assigned this JIRA to you. Thanks for providing a patch.

          Show
          liuml07 Mingliang Liu added a comment - Santhosh G Nayak , I added you to the Hadoop Common contributor list and assigned this JIRA to you. Thanks for providing a patch.
          Hide
          snayak Santhosh G Nayak added a comment -

          Attaching initial version of the patch containing proposed changes.

          Show
          snayak Santhosh G Nayak added a comment - Attaching initial version of the patch containing proposed changes.

            People

            • Assignee:
              snayak Santhosh G Nayak
              Reporter:
              snayak Santhosh G Nayak
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development