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

WASB Logging: Improve WASB Logging around deletes, reads and writes

    Details

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

      Description

      Logging around the WASB component is very limited and it is disabled by default. This improvement is created to add logging around Reads, Writes and Deletes when Azure Storage Exception to capture the blobs that hit the exception. This information is useful while communicating with the Azure storage team for debugging purposes.

      1. 0001-HADOOP-12350-Added-WASB-Logging-Statement.patch
        8 kB
        Dushyanth
      2. 0001-HADOOP-12350-Moving-from-commons.logging-to-slf4j-lo.patch
        26 kB
        Dushyanth
      3. 0001-HADOOP-12350-Improve-WASB-Logging-Itr3.patch
        26 kB
        Dushyanth
      4. HADOOP-12350.004.patch
        32 kB
        Dushyanth
      5. HADOOP-12350.005.patch
        32 kB
        Dushyanth
      6. HADOOP-12350.006.patch
        32 kB
        Dushyanth
      7. HADOOP-12350.007.patch
        32 kB
        Dushyanth

        Activity

        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #459 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/459/)
        HADOOP-12350. WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636)

        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #459 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/459/ ) HADOOP-12350 . WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java hadoop-common-project/hadoop-common/CHANGES.txt hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2398 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2398/)
        HADOOP-12350. WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2398 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2398/ ) HADOOP-12350 . WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #484 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/484/)
        HADOOP-12350. WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636)

        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #484 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/484/ ) HADOOP-12350 . WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java hadoop-common-project/hadoop-common/CHANGES.txt hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk #2428 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2428/)
        HADOOP-12350. WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636)

        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #2428 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2428/ ) HADOOP-12350 . WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java hadoop-common-project/hadoop-common/CHANGES.txt hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #493 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/493/)
        HADOOP-12350. WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636)

        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #493 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/493/ ) HADOOP-12350 . WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java hadoop-common-project/hadoop-common/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk #1223 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1223/)
        HADOOP-12350. WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636)

        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #1223 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1223/ ) HADOOP-12350 . WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java hadoop-common-project/hadoop-common/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #8574 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8574/)
        HADOOP-12350. WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636)

        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
        • hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #8574 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8574/ ) HADOOP-12350 . WASB Logging: Improve WASB Logging around deletes, reads (cnauroth: rev 5f6edb30c2bb648d5564c951edc25645e17e6636) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java hadoop-common-project/hadoop-common/CHANGES.txt
        Hide
        cnauroth Chris Nauroth added a comment -

        +1 for patch v007. I confirmed once again that the test failure was unrelated and already tracked in existing JIRAs. I have committed this to trunk and branch-2. Dushyanth, thank you for the contribution.

        Show
        cnauroth Chris Nauroth added a comment - +1 for patch v007. I confirmed once again that the test failure was unrelated and already tracked in existing JIRAs. I have committed this to trunk and branch-2. Dushyanth , thank you for the contribution.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 16m 39s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 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 javac 8m 5s There were no new javac warning messages.
        +1 javadoc 10m 20s There were no new javadoc warning messages.
        -1 release audit 0m 16s The applied patch generated 1 release audit warnings.
        +1 checkstyle 0m 21s There were no new checkstyle issues.
        +1 whitespace 0m 1s The patch has no lines that end in whitespace.
        +1 install 1m 30s mvn install still works.
        +1 eclipse:eclipse 0m 38s The patch built with eclipse:eclipse.
        +1 findbugs 0m 47s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        -1 tools/hadoop tests 1m 10s Tests failed in hadoop-azure.
            39m 51s  



        Reason Tests
        Failed unit tests hadoop.fs.azure.TestNativeAzureFileSystemOperationsMocked



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12765056/HADOOP-12350.007.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / 2fc2b50
        Release Audit https://builds.apache.org/job/PreCommit-HADOOP-Build/7769/artifact/patchprocess/patchReleaseAuditProblems.txt
        hadoop-azure test log https://builds.apache.org/job/PreCommit-HADOOP-Build/7769/artifact/patchprocess/testrun_hadoop-azure.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/7769/testReport/
        Java 1.7.0_55
        uname Linux asf901.gq1.ygridcore.net 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
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/7769/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 16m 39s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 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 javac 8m 5s There were no new javac warning messages. +1 javadoc 10m 20s There were no new javadoc warning messages. -1 release audit 0m 16s The applied patch generated 1 release audit warnings. +1 checkstyle 0m 21s There were no new checkstyle issues. +1 whitespace 0m 1s The patch has no lines that end in whitespace. +1 install 1m 30s mvn install still works. +1 eclipse:eclipse 0m 38s The patch built with eclipse:eclipse. +1 findbugs 0m 47s The patch does not introduce any new Findbugs (version 3.0.0) warnings. -1 tools/hadoop tests 1m 10s Tests failed in hadoop-azure.     39m 51s   Reason Tests Failed unit tests hadoop.fs.azure.TestNativeAzureFileSystemOperationsMocked Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12765056/HADOOP-12350.007.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 2fc2b50 Release Audit https://builds.apache.org/job/PreCommit-HADOOP-Build/7769/artifact/patchprocess/patchReleaseAuditProblems.txt hadoop-azure test log https://builds.apache.org/job/PreCommit-HADOOP-Build/7769/artifact/patchprocess/testrun_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/7769/testReport/ Java 1.7.0_55 uname Linux asf901.gq1.ygridcore.net 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 Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/7769/console This message was automatically generated.
        Hide
        dchickabasapa Dushyanth added a comment -

        Attaching with the tab character style check warning fix.

        Show
        dchickabasapa Dushyanth added a comment - Attaching with the tab character style check warning fix.
        Hide
        cnauroth Chris Nauroth added a comment -

        Dushyanth, there is one more tab character to clear up. Thanks!

        ./hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java:2440:1: Line contains a tab character.
        
        Show
        cnauroth Chris Nauroth added a comment - Dushyanth , there is one more tab character to clear up. Thanks! ./hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java:2440:1: Line contains a tab character.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 16m 2s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 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 javac 7m 48s There were no new javac warning messages.
        +1 javadoc 10m 6s There were no new javadoc warning messages.
        -1 release audit 0m 16s The applied patch generated 1 release audit warnings.
        -1 checkstyle 0m 21s The applied patch generated 1 new checkstyle issues (total was 59, now 60).
        +1 whitespace 0m 0s The patch has no lines that end in whitespace.
        +1 install 1m 30s mvn install still works.
        +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
        +1 findbugs 0m 47s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        -1 tools/hadoop tests 1m 11s Tests failed in hadoop-azure.
            38m 38s  



        Reason Tests
        Failed unit tests hadoop.fs.azure.TestNativeAzureFileSystemOperationsMocked



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12764947/HADOOP-12350.006.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / 3b85bd7
        Release Audit https://builds.apache.org/job/PreCommit-HADOOP-Build/7767/artifact/patchprocess/patchReleaseAuditProblems.txt
        checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/7767/artifact/patchprocess/diffcheckstylehadoop-azure.txt
        hadoop-azure test log https://builds.apache.org/job/PreCommit-HADOOP-Build/7767/artifact/patchprocess/testrun_hadoop-azure.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/7767/testReport/
        Java 1.7.0_55
        uname Linux asf906.gq1.ygridcore.net 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
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/7767/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 16m 2s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 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 javac 7m 48s There were no new javac warning messages. +1 javadoc 10m 6s There were no new javadoc warning messages. -1 release audit 0m 16s The applied patch generated 1 release audit warnings. -1 checkstyle 0m 21s The applied patch generated 1 new checkstyle issues (total was 59, now 60). +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 install 1m 30s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 0m 47s The patch does not introduce any new Findbugs (version 3.0.0) warnings. -1 tools/hadoop tests 1m 11s Tests failed in hadoop-azure.     38m 38s   Reason Tests Failed unit tests hadoop.fs.azure.TestNativeAzureFileSystemOperationsMocked Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12764947/HADOOP-12350.006.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 3b85bd7 Release Audit https://builds.apache.org/job/PreCommit-HADOOP-Build/7767/artifact/patchprocess/patchReleaseAuditProblems.txt checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/7767/artifact/patchprocess/diffcheckstylehadoop-azure.txt hadoop-azure test log https://builds.apache.org/job/PreCommit-HADOOP-Build/7767/artifact/patchprocess/testrun_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/7767/testReport/ Java 1.7.0_55 uname Linux asf906.gq1.ygridcore.net 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 Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/7767/console This message was automatically generated.
        Hide
        dchickabasapa Dushyanth added a comment -

        Chris Nauroth Sorry for the additional iteration needed here. The line numbers didnt match so I probably missed the warnings. I have addressed the comments and added a new iteration.

        Show
        dchickabasapa Dushyanth added a comment - Chris Nauroth Sorry for the additional iteration needed here. The line numbers didnt match so I probably missed the warnings. I have addressed the comments and added a new iteration.
        Hide
        dchickabasapa Dushyanth added a comment -

        Addressing the stylecheck warnings from the previous iteration.

        Show
        dchickabasapa Dushyanth added a comment - Addressing the stylecheck warnings from the previous iteration.
        Hide
        cnauroth Chris Nauroth added a comment -

        Dushyanth, it's so close! Sorry to nitpick, but please take a look at the 3 remaining checkstyle warnings and upload one more revision of the patch to address them. Thanks again.

        Show
        cnauroth Chris Nauroth added a comment - Dushyanth , it's so close! Sorry to nitpick, but please take a look at the 3 remaining checkstyle warnings and upload one more revision of the patch to address them. Thanks again.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 16m 12s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 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 javac 7m 47s There were no new javac warning messages.
        +1 javadoc 10m 3s There were no new javadoc warning messages.
        -1 release audit 0m 17s The applied patch generated 1 release audit warnings.
        -1 checkstyle 0m 22s The applied patch generated 3 new checkstyle issues (total was 59, now 62).
        +1 whitespace 0m 1s The patch has no lines that end in whitespace.
        +1 install 1m 29s mvn install still works.
        +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
        +1 findbugs 0m 47s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        -1 tools/hadoop tests 1m 11s Tests failed in hadoop-azure.
            38m 46s  



        Reason Tests
        Failed unit tests hadoop.fs.azure.TestNativeAzureFileSystemOperationsMocked



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12764918/HADOOP-12350.005.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / 3b85bd7
        Release Audit https://builds.apache.org/job/PreCommit-HADOOP-Build/7764/artifact/patchprocess/patchReleaseAuditProblems.txt
        checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/7764/artifact/patchprocess/diffcheckstylehadoop-azure.txt
        hadoop-azure test log https://builds.apache.org/job/PreCommit-HADOOP-Build/7764/artifact/patchprocess/testrun_hadoop-azure.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/7764/testReport/
        Java 1.7.0_55
        uname Linux asf906.gq1.ygridcore.net 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
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/7764/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 16m 12s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 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 javac 7m 47s There were no new javac warning messages. +1 javadoc 10m 3s There were no new javadoc warning messages. -1 release audit 0m 17s The applied patch generated 1 release audit warnings. -1 checkstyle 0m 22s The applied patch generated 3 new checkstyle issues (total was 59, now 62). +1 whitespace 0m 1s The patch has no lines that end in whitespace. +1 install 1m 29s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 0m 47s The patch does not introduce any new Findbugs (version 3.0.0) warnings. -1 tools/hadoop tests 1m 11s Tests failed in hadoop-azure.     38m 46s   Reason Tests Failed unit tests hadoop.fs.azure.TestNativeAzureFileSystemOperationsMocked Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12764918/HADOOP-12350.005.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 3b85bd7 Release Audit https://builds.apache.org/job/PreCommit-HADOOP-Build/7764/artifact/patchprocess/patchReleaseAuditProblems.txt checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/7764/artifact/patchprocess/diffcheckstylehadoop-azure.txt hadoop-azure test log https://builds.apache.org/job/PreCommit-HADOOP-Build/7764/artifact/patchprocess/testrun_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/7764/testReport/ Java 1.7.0_55 uname Linux asf906.gq1.ygridcore.net 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 Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/7764/console This message was automatically generated.
        Hide
        dchickabasapa Dushyanth added a comment -

        Added a new patch that has the fix for the checkstyle and whitespace warnings.

        Show
        dchickabasapa Dushyanth added a comment - Added a new patch that has the fix for the checkstyle and whitespace warnings.
        Hide
        cnauroth Chris Nauroth added a comment -

        The test failure is unrelated and tracked separately in HADOOP-12445 and HDFS-9187.

        The release audit warning is unrelated and tracked in HDFS-9182.

        Dushyanth, the patch looks good overall. I ran tests locally, including integration tests against my own Azure storage account. I think this will be ready to commit after one more revision to clean up the checkstyle and whitespace warnings. Thanks!

        Show
        cnauroth Chris Nauroth added a comment - The test failure is unrelated and tracked separately in HADOOP-12445 and HDFS-9187 . The release audit warning is unrelated and tracked in HDFS-9182 . Dushyanth , the patch looks good overall. I ran tests locally, including integration tests against my own Azure storage account. I think this will be ready to commit after one more revision to clean up the checkstyle and whitespace warnings. Thanks!
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 16m 31s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 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 javac 8m 11s There were no new javac warning messages.
        +1 javadoc 10m 7s There were no new javadoc warning messages.
        -1 release audit 0m 15s The applied patch generated 1 release audit warnings.
        -1 checkstyle 0m 21s The applied patch generated 20 new checkstyle issues (total was 59, now 79).
        -1 whitespace 0m 1s The patch has 4 line(s) that end in whitespace. Use git apply --whitespace=fix.
        +1 install 1m 28s mvn install still works.
        +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
        +1 findbugs 0m 48s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        -1 tools/hadoop tests 1m 11s Tests failed in hadoop-azure.
            39m 32s  



        Reason Tests
        Failed unit tests hadoop.fs.azure.TestNativeAzureFileSystemOperationsMocked



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12764661/HADOOP-12350.004.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / fd026f5
        Release Audit https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/artifact/patchprocess/patchReleaseAuditProblems.txt
        checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/artifact/patchprocess/diffcheckstylehadoop-azure.txt
        whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/artifact/patchprocess/whitespace.txt
        hadoop-azure test log https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/artifact/patchprocess/testrun_hadoop-azure.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/testReport/
        Java 1.7.0_55
        uname Linux asf901.gq1.ygridcore.net 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
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 16m 31s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 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 javac 8m 11s There were no new javac warning messages. +1 javadoc 10m 7s There were no new javadoc warning messages. -1 release audit 0m 15s The applied patch generated 1 release audit warnings. -1 checkstyle 0m 21s The applied patch generated 20 new checkstyle issues (total was 59, now 79). -1 whitespace 0m 1s The patch has 4 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 28s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 0m 48s The patch does not introduce any new Findbugs (version 3.0.0) warnings. -1 tools/hadoop tests 1m 11s Tests failed in hadoop-azure.     39m 32s   Reason Tests Failed unit tests hadoop.fs.azure.TestNativeAzureFileSystemOperationsMocked Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12764661/HADOOP-12350.004.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / fd026f5 Release Audit https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/artifact/patchprocess/patchReleaseAuditProblems.txt checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/artifact/patchprocess/diffcheckstylehadoop-azure.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/artifact/patchprocess/whitespace.txt hadoop-azure test log https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/artifact/patchprocess/testrun_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/testReport/ Java 1.7.0_55 uname Linux asf901.gq1.ygridcore.net 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 Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/7752/console This message was automatically generated.
        Hide
        dchickabasapa Dushyanth added a comment -

        Adding the patch after rebasing to the latest on Trunk

        Show
        dchickabasapa Dushyanth added a comment - Adding the patch after rebasing to the latest on Trunk
        Hide
        dchickabasapa Dushyanth added a comment -

        Chris Nauroth Thanks for the review. I have added a new iteration addressing your comments in the previous review.

        Please take a look.

        Show
        dchickabasapa Dushyanth added a comment - Chris Nauroth Thanks for the review. I have added a new iteration addressing your comments in the previous review. Please take a look.
        Hide
        cnauroth Chris Nauroth added a comment -

        Hi Dushyanth. Regarding changes like this:

        -        if (LOG.isDebugEnabled()) {
        -          LOG.debug("Found " + key
        +        LOG.debug("Found " + key
                       + " as an explicit blob. Checking if it's a file or folder.");
        -        }
        

        Instead, the idiomatic way to do this with slf4j is:

                LOG.debug("Found {} as an explicit blob. Checking if it's a file or folder.", key);
        

        The "{}" is a placeholder in slf4j syntax, The logging methods (info, debug, etc.) are variadic, so you'd pass exactly one argument for each "{}" placeholder in the format string. Internally, slf4j only does the placeholder replacement if the logging level is enabled. This is why we no longer need the explicit checks for isDebugEnabled(), but we do need to make sure we're using slf4j as I described to avoid unnecessary string concatenation cost.

        If this is unclear, I recommend reviewing a few of the recent slf4j migration patches that have already gone into the codebase.

        Show
        cnauroth Chris Nauroth added a comment - Hi Dushyanth . Regarding changes like this: - if (LOG.isDebugEnabled()) { - LOG.debug( "Found " + key + LOG.debug( "Found " + key + " as an explicit blob. Checking if it's a file or folder." ); - } Instead, the idiomatic way to do this with slf4j is: LOG.debug( "Found {} as an explicit blob. Checking if it's a file or folder." , key); The "{}" is a placeholder in slf4j syntax, The logging methods ( info , debug , etc.) are variadic, so you'd pass exactly one argument for each "{}" placeholder in the format string. Internally, slf4j only does the placeholder replacement if the logging level is enabled. This is why we no longer need the explicit checks for isDebugEnabled() , but we do need to make sure we're using slf4j as I described to avoid unnecessary string concatenation cost. If this is unclear, I recommend reviewing a few of the recent slf4j migration patches that have already gone into the codebase.
        Hide
        dchickabasapa Dushyanth added a comment -

        Chris Nauroth Thanks for the input chris. I have added a patch that moves the logging framework to Slf4j. As discussed in the JIRA I have added a private cleanup method inside NativeAzureFileSystem class.

        To address the comment on AzureNativeFileSystemStore, yes the log statement is specifically for fatal exception that can't be retried again.

        Show
        dchickabasapa Dushyanth added a comment - Chris Nauroth Thanks for the input chris. I have added a patch that moves the logging framework to Slf4j. As discussed in the JIRA I have added a private cleanup method inside NativeAzureFileSystem class. To address the comment on AzureNativeFileSystemStore, yes the log statement is specifically for fatal exception that can't be retried again.
        Hide
        dchickabasapa Dushyanth added a comment -

        Adding patch that changes to slf4j logging framework.

        Show
        dchickabasapa Dushyanth added a comment - Adding patch that changes to slf4j logging framework.
        Hide
        cnauroth Chris Nauroth added a comment -

        considering that Hadoop community is moving towards use of Slf4j, would the IOUtils be exposing an Api that that takes Slf4j?

        I think that makes sense, but adding an overload of IOUtils#cleanup would require some wider cleanup across various sub-modules. (If a caller explicitly passes null, then Java can't decide whether to dispatch to the Commons Logging version or the slf4j version.)

        That also makes me wonder if it would impact downstream projects. IOUtils is annotated Public.

        Let's not try to do this within the scope of the current patch.

        Do you see issues in having this implemented on the NativeAzureFileSystem class itself and move away from using IOUtils?

        I think it's fine to have a private cleanup helper method for now. It can be refactored later when we figure out the best way to put a helper method in Hadoop Common.

        Show
        cnauroth Chris Nauroth added a comment - considering that Hadoop community is moving towards use of Slf4j, would the IOUtils be exposing an Api that that takes Slf4j? I think that makes sense, but adding an overload of IOUtils#cleanup would require some wider cleanup across various sub-modules. (If a caller explicitly passes null , then Java can't decide whether to dispatch to the Commons Logging version or the slf4j version.) That also makes me wonder if it would impact downstream projects. IOUtils is annotated Public . Let's not try to do this within the scope of the current patch. Do you see issues in having this implemented on the NativeAzureFileSystem class itself and move away from using IOUtils? I think it's fine to have a private cleanup helper method for now. It can be refactored later when we figure out the best way to put a helper method in Hadoop Common.
        Hide
        dchickabasapa Dushyanth added a comment -

        Chris Nauroth Thanks for the review Chris.

        I see that inside NativeAzureFileSystem class there are calls to IOUtils.cleanup(LOG, out). However IOUtils only exposes cleanup API that take Commons Logging objects and not Slf4j objects.

        considering that Hadoop community is moving towards use of Slf4j, would the IOUtils be exposing an Api that that takes Slf4j?

        If not, then I looked at the code for cleanup, it basically closes
        I looked at the code for cleanup it basically closes the Closeable objects and logs exception message on it. Do you see issues in having this implemented on the NativeAzureFileSystem class itself and move away from using IOUtils?

        Show
        dchickabasapa Dushyanth added a comment - Chris Nauroth Thanks for the review Chris. I see that inside NativeAzureFileSystem class there are calls to IOUtils.cleanup(LOG, out). However IOUtils only exposes cleanup API that take Commons Logging objects and not Slf4j objects. considering that Hadoop community is moving towards use of Slf4j, would the IOUtils be exposing an Api that that takes Slf4j? If not, then I looked at the code for cleanup, it basically closes I looked at the code for cleanup it basically closes the Closeable objects and logs exception message on it. Do you see issues in having this implemented on the NativeAzureFileSystem class itself and move away from using IOUtils?
        Hide
        cnauroth Chris Nauroth added a comment -

        Hello Dushyanth. Thank you for the patch.

        I think this is a good opportunity to convert this class to use slf4j. If you grep for slf4j throughout the Hadoop source, you'll be able to find existing examples that use it. Doing the conversion should just be a matter of removing the Commons Logging imports, adding the slf4j imports, and then making a small change to the LOG initialization to call slf4j's LoggerFactory#getLogger.

        After that, the logic of this patch can be simplified by relying on slf4j's improved API. For example, this code...

              if (LOG.isDebugEnabled()) {
                LOG.error("Encountered Storage Exception for delete on Blob : "
                + blob.getUri() + " Exception Details : " + e.getMessage() +
                " Error Code : " + e.getErrorCode());
              }
        

        ...can be simplified to this...

              LOG.debug("Encountered Storage Exception for delete on Blob : {} "
                  + "Exception Details : {} Error Code : {}", blob.getUri(),
                  e.getMessage(), e.getErrorCode());
        

        There won't be a need for the isDebugEnabled checks. Also, notice how the original code sample from the patch checked for debug level, but then logged at error level. The slf4j API helps eliminate that class of bugs.

        For the change in AzureNativeFileSystemStore, there is a debug-level logging statement in the same method that logs exceptions being swallowed during retries. Should this new log statement only apply when it's a fatal exception that can't be retried again?

        Show
        cnauroth Chris Nauroth added a comment - Hello Dushyanth . Thank you for the patch. I think this is a good opportunity to convert this class to use slf4j. If you grep for slf4j throughout the Hadoop source, you'll be able to find existing examples that use it. Doing the conversion should just be a matter of removing the Commons Logging imports, adding the slf4j imports, and then making a small change to the LOG initialization to call slf4j's LoggerFactory#getLogger . After that, the logic of this patch can be simplified by relying on slf4j's improved API. For example, this code... if (LOG.isDebugEnabled()) { LOG.error( "Encountered Storage Exception for delete on Blob : " + blob.getUri() + " Exception Details : " + e.getMessage() + " Error Code : " + e.getErrorCode()); } ...can be simplified to this... LOG.debug( "Encountered Storage Exception for delete on Blob : {} " + "Exception Details : {} Error Code : {}" , blob.getUri(), e.getMessage(), e.getErrorCode()); There won't be a need for the isDebugEnabled checks. Also, notice how the original code sample from the patch checked for debug level, but then logged at error level. The slf4j API helps eliminate that class of bugs. For the change in AzureNativeFileSystemStore , there is a debug-level logging statement in the same method that logs exceptions being swallowed during retries. Should this new log statement only apply when it's a fatal exception that can't be retried again?
        Hide
        dchickabasapa Dushyanth added a comment -

        Added first iteration. Patch contains:

        1) Logging statement around Read, Write and Deletes.
        2) Removed un-necessary imports

        Show
        dchickabasapa Dushyanth added a comment - Added first iteration. Patch contains: 1) Logging statement around Read, Write and Deletes. 2) Removed un-necessary imports

          People

          • Assignee:
            dchickabasapa Dushyanth
            Reporter:
            dchickabasapa Dushyanth
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development