Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-11832

Switch leftover logs to slf4j format in BlockManager.java

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.7.0, 2.8.0, 3.0.0-alpha1
    • Fix Version/s: 3.0.0-alpha4
    • Component/s: namenode
    • Labels:
      None

      Description

      HDFS-7706 Switch BlockManager logging to use slf4j. But the logging formats were not modified appropriately. For example:
      if (LOG.isDebugEnabled())

      { LOG.debug("blocks = " + java.util.Arrays.asList(blocks)); }

      These codes should be modified to:
      LOG.debug("blocks = {}", java.util.Arrays.asList(blocks));

      1. HDFS-11832.001.patch
        16 kB
        Chen Liang
      2. HDFS-11832.002.patch
        16 kB
        Hui Xu
      3. HDFS-11832.003.patch
        18 kB
        Hui Xu
      4. HDFS-11832.004.patch
        18 kB
        Chen Liang
      5. HDFS-11832.005.patch
        18 kB
        Hui Xu

        Activity

        Hide
        vagarychen Chen Liang added a comment - - edited

        Post v001 patch to change the usage of log in BlockManager

        Show
        vagarychen Chen Liang added a comment - - edited Post v001 patch to change the usage of log in BlockManager
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 34s 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 14m 17s trunk passed
        +1 compile 0m 46s trunk passed
        +1 checkstyle 0m 38s trunk passed
        +1 mvnsite 0m 54s trunk passed
        +1 mvneclipse 0m 15s trunk passed
        +1 findbugs 1m 42s trunk passed
        +1 javadoc 0m 43s trunk passed
        +1 mvninstall 0m 52s the patch passed
        +1 compile 0m 48s the patch passed
        +1 javac 0m 48s the patch passed
        -0 checkstyle 0m 35s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 120 unchanged - 1 fixed = 122 total (was 121)
        +1 mvnsite 0m 53s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 44s the patch passed
        +1 javadoc 0m 39s the patch passed
        -1 unit 97m 10s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 24s The patch does not generate ASF License warnings.
        124m 39s



        Reason Tests
        Failed junit tests hadoop.hdfs.server.namenode.TestReconstructStripedBlocks
          hadoop.hdfs.server.blockmanagement.TestReconstructStripedBlocksWithRackAwareness



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:14b5c93
        JIRA Issue HDFS-11832
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12868384/HDFS-11832.001.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 34eb50580bbc 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / b415c6f
        Default Java 1.8.0_131
        findbugs v3.1.0-RC1
        checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/19454/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/19454/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19454/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19454/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 34s 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 14m 17s trunk passed +1 compile 0m 46s trunk passed +1 checkstyle 0m 38s trunk passed +1 mvnsite 0m 54s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 1m 42s trunk passed +1 javadoc 0m 43s trunk passed +1 mvninstall 0m 52s the patch passed +1 compile 0m 48s the patch passed +1 javac 0m 48s the patch passed -0 checkstyle 0m 35s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 120 unchanged - 1 fixed = 122 total (was 121) +1 mvnsite 0m 53s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 44s the patch passed +1 javadoc 0m 39s the patch passed -1 unit 97m 10s hadoop-hdfs in the patch failed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 124m 39s Reason Tests Failed junit tests hadoop.hdfs.server.namenode.TestReconstructStripedBlocks   hadoop.hdfs.server.blockmanagement.TestReconstructStripedBlocksWithRackAwareness Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HDFS-11832 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12868384/HDFS-11832.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 34eb50580bbc 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / b415c6f Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/19454/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/19454/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19454/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19454/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        10075197 Hui Xu added a comment -

        Thank Chen Liang for providing the patch timely.
        Just I found there are still some logs need to modify the format according to slf4j.
        So I attach the HDFS-11832.002.patch.

        Show
        10075197 Hui Xu added a comment - Thank Chen Liang for providing the patch timely. Just I found there are still some logs need to modify the format according to slf4j. So I attach the HDFS-11832 .002.patch.
        Hide
        10075197 Hui Xu added a comment -

        Hi,
        slf4j need not such boiler-plate code as:
        if (LOG.isDebugEnabled()) {

        So I attached HDFS-11832.002.patch.

        Show
        10075197 Hui Xu added a comment - Hi, slf4j need not such boiler-plate code as: if (LOG.isDebugEnabled()) { So I attached HDFS-11832 .002.patch.
        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 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 14m 47s trunk passed
        +1 compile 0m 49s trunk passed
        +1 checkstyle 0m 37s trunk passed
        +1 mvnsite 0m 56s trunk passed
        +1 mvneclipse 0m 15s trunk passed
        +1 findbugs 1m 46s trunk passed
        +1 javadoc 0m 42s trunk passed
        +1 mvninstall 0m 52s the patch passed
        +1 compile 0m 46s the patch passed
        +1 javac 0m 46s the patch passed
        -0 checkstyle 0m 35s hadoop-hdfs-project/hadoop-hdfs: The patch generated 5 new + 114 unchanged - 2 fixed = 119 total (was 116)
        +1 mvnsite 0m 50s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 49s the patch passed
        +1 javadoc 0m 38s the patch passed
        -1 unit 70m 14s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        97m 57s



        Reason Tests
        Failed junit tests hadoop.hdfs.TestMaintenanceState
          hadoop.hdfs.TestDFSStripedOutputStreamWithFailure150
          hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure
          hadoop.hdfs.server.datanode.TestDirectoryScanner
          hadoop.hdfs.server.namenode.TestDecommissioningStatus
          hadoop.hdfs.TestDFSRSDefault10x4StripedOutputStreamWithFailure
          hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureToleration



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:14b5c93
        JIRA Issue HDFS-11832
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12868454/HDFS-11832.002.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux e155db9e88bb 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 / d87a63a
        Default Java 1.8.0_131
        findbugs v3.1.0-RC1
        checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/19457/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/19457/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19457/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19457/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 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 14m 47s trunk passed +1 compile 0m 49s trunk passed +1 checkstyle 0m 37s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 1m 46s trunk passed +1 javadoc 0m 42s trunk passed +1 mvninstall 0m 52s the patch passed +1 compile 0m 46s the patch passed +1 javac 0m 46s the patch passed -0 checkstyle 0m 35s hadoop-hdfs-project/hadoop-hdfs: The patch generated 5 new + 114 unchanged - 2 fixed = 119 total (was 116) +1 mvnsite 0m 50s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 49s the patch passed +1 javadoc 0m 38s the patch passed -1 unit 70m 14s hadoop-hdfs in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 97m 57s Reason Tests Failed junit tests hadoop.hdfs.TestMaintenanceState   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure150   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure   hadoop.hdfs.server.datanode.TestDirectoryScanner   hadoop.hdfs.server.namenode.TestDecommissioningStatus   hadoop.hdfs.TestDFSRSDefault10x4StripedOutputStreamWithFailure   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureToleration Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HDFS-11832 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12868454/HDFS-11832.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux e155db9e88bb 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 / d87a63a Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/19457/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/19457/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19457/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19457/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        vagarychen Chen Liang added a comment -

        Thanks Hui Xu for updating the patch! I tested v002 patch locally and all tests passed, so the failed tests should be unrelated.

        Show
        vagarychen Chen Liang added a comment - Thanks Hui Xu for updating the patch! I tested v002 patch locally and all tests passed, so the failed tests should be unrelated.
        Hide
        10075197 Hui Xu added a comment -

        Attach v003 patch to fix the LineLength warning and include the modification of InvalidateBlocks.java contributed by Chen Liang. Thanks!

        Show
        10075197 Hui Xu added a comment - Attach v003 patch to fix the LineLength warning and include the modification of InvalidateBlocks.java contributed by Chen Liang. Thanks!
        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 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 15m 11s trunk passed
        +1 compile 0m 48s trunk passed
        +1 checkstyle 0m 37s trunk passed
        +1 mvnsite 0m 52s trunk passed
        +1 mvneclipse 0m 14s trunk passed
        +1 findbugs 1m 39s trunk passed
        +1 javadoc 0m 39s trunk passed
        +1 mvninstall 0m 50s the patch passed
        +1 compile 0m 46s the patch passed
        +1 javac 0m 46s the patch passed
        +1 checkstyle 0m 34s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 119 unchanged - 2 fixed = 119 total (was 121)
        +1 mvnsite 0m 51s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 45s the patch passed
        +1 javadoc 0m 38s the patch passed
        -1 unit 71m 44s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        99m 28s



        Reason Tests
        Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting
          hadoop.hdfs.TestDFSStripedOutputStreamWithFailure140
          hadoop.hdfs.TestDFSStripedOutputStreamWithFailure080
          hadoop.hdfs.TestDFSRSDefault10x4StripedOutputStreamWithFailure



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:14b5c93
        JIRA Issue HDFS-11832
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12868662/HDFS-11832.003.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 23d855d5711a 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 / 44e9ef2
        Default Java 1.8.0_131
        findbugs v3.1.0-RC1
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/19485/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19485/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19485/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 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 15m 11s trunk passed +1 compile 0m 48s trunk passed +1 checkstyle 0m 37s trunk passed +1 mvnsite 0m 52s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 39s trunk passed +1 javadoc 0m 39s trunk passed +1 mvninstall 0m 50s the patch passed +1 compile 0m 46s the patch passed +1 javac 0m 46s the patch passed +1 checkstyle 0m 34s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 119 unchanged - 2 fixed = 119 total (was 121) +1 mvnsite 0m 51s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 45s the patch passed +1 javadoc 0m 38s the patch passed -1 unit 71m 44s hadoop-hdfs in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 99m 28s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure140   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure080   hadoop.hdfs.TestDFSRSDefault10x4StripedOutputStreamWithFailure Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HDFS-11832 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12868662/HDFS-11832.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 23d855d5711a 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 / 44e9ef2 Default Java 1.8.0_131 findbugs v3.1.0-RC1 unit https://builds.apache.org/job/PreCommit-HDFS-Build/19485/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19485/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19485/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        ajisakaa Akira Ajisaka added a comment -

        Some source code still need if(LOG.isDebugEnabled()). For example, the following source code

          LOG.debug("blocks = {}", java.util.Arrays.asList(blocks));
        

        creates additional ArrayList instance by Arrays.asList regardless of the log level, so the code should be guarded.
        This applies to

              if (LOG.isDebugEnabled()) {
                LOG.debug("Initial report of block " + iblk.getBlockName()
                    + " on " + storageInfo.getDatanodeDescriptor() + " size " +
                    iblk.getNumBytes() + " replicaState = " + reportedState);
              }
        

        as well.

        Show
        ajisakaa Akira Ajisaka added a comment - Some source code still need if(LOG.isDebugEnabled()) . For example, the following source code LOG.debug( "blocks = {}" , java.util.Arrays.asList(blocks)); creates additional ArrayList instance by Arrays.asList regardless of the log level, so the code should be guarded. This applies to if (LOG.isDebugEnabled()) { LOG.debug( "Initial report of block " + iblk.getBlockName() + " on " + storageInfo.getDatanodeDescriptor() + " size " + iblk.getNumBytes() + " replicaState = " + reportedState); } as well.
        Hide
        vagarychen Chen Liang added a comment -

        Thanks Hui Xu for the patches! and thanks Akira Ajisaka for the comments!

        Post v004 patch to add back guard to a few places where there seem to be potentially expensive operations. This is based on Hui Xu's v003 patch.

        Show
        vagarychen Chen Liang added a comment - Thanks Hui Xu for the patches! and thanks Akira Ajisaka for the comments! Post v004 patch to add back guard to a few places where there seem to be potentially expensive operations. This is based on Hui Xu's v003 patch.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 18s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 mvninstall 13m 37s trunk passed
        +1 compile 0m 51s trunk passed
        +1 checkstyle 0m 37s trunk passed
        +1 mvnsite 0m 54s trunk passed
        +1 mvneclipse 0m 15s trunk passed
        +1 findbugs 1m 43s trunk passed
        +1 javadoc 0m 41s trunk passed
        +1 mvninstall 0m 49s the patch passed
        +1 compile 0m 47s the patch passed
        +1 javac 0m 47s the patch passed
        +1 checkstyle 0m 36s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 119 unchanged - 2 fixed = 119 total (was 121)
        +1 mvnsite 0m 53s the patch passed
        +1 mvneclipse 0m 11s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 50s the patch passed
        +1 javadoc 0m 39s the patch passed
        -1 unit 66m 8s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        92m 27s



        Reason Tests
        Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure150
          hadoop.hdfs.tools.TestDFSZKFailoverController
          hadoop.hdfs.TestDFSRSDefault10x4StripedOutputStreamWithFailure
          hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:14b5c93
        JIRA Issue HDFS-11832
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12868803/HDFS-11832.004.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 0cb6983da7ea 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 / 40e6a85
        Default Java 1.8.0_131
        findbugs v3.1.0-RC1
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/19495/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19495/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19495/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 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 13m 37s trunk passed +1 compile 0m 51s trunk passed +1 checkstyle 0m 37s trunk passed +1 mvnsite 0m 54s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 1m 43s trunk passed +1 javadoc 0m 41s trunk passed +1 mvninstall 0m 49s the patch passed +1 compile 0m 47s the patch passed +1 javac 0m 47s the patch passed +1 checkstyle 0m 36s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 119 unchanged - 2 fixed = 119 total (was 121) +1 mvnsite 0m 53s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 50s the patch passed +1 javadoc 0m 39s the patch passed -1 unit 66m 8s hadoop-hdfs in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 92m 27s Reason Tests Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure150   hadoop.hdfs.tools.TestDFSZKFailoverController   hadoop.hdfs.TestDFSRSDefault10x4StripedOutputStreamWithFailure   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HDFS-11832 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12868803/HDFS-11832.004.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 0cb6983da7ea 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 / 40e6a85 Default Java 1.8.0_131 findbugs v3.1.0-RC1 unit https://builds.apache.org/job/PreCommit-HDFS-Build/19495/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19495/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19495/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        10075197 Hui Xu added a comment -

        Hi Akira Ajisaka and Chen Liang,
        Would you refer to the official link as follows?

        https://www.slf4j.org/faq.html#logging_performance

        Better yet, use parameterized messages
        There exists a very convenient alternative based on message formats. Assuming entry is an object, you can write:
        Object entry = new SomeObject();
        logger.debug("The entry is {}.", entry);
        After evaluating whether to log or not, and only if the decision is affirmative, will the logger implementation format the message and replace the '{}' pair with the string value of entry. In other words, this form does not incur the cost of parameter construction in case the log statement is disabled.
        The following two lines will yield the exact same output. However, the second form will outperform the first form by a factor of at least 30, in case of a disabled logging statement.
        logger.debug("The new entry is "entry".");
        logger.debug("The new entry is {}.", entry);

        And this is the source code:
        public void debug(String format, Object arg) {
        if(this.logger.isDebugEnabled())

        { FormattingTuple ft = MessageFormatter.format(format, arg); this.logger.log(FQCN, Level.DEBUG, ft.getMessage(), ft.getThrowable()); }

        }

        public static final FormattingTuple format(String messagePattern, Object arg) {
        return arrayFormat(messagePattern, new Object[]

        {arg}

        );
        }

        So, Arrays.asList(blocks) will not new the object until the format function.
        Looking forward to your next discussion.

        Show
        10075197 Hui Xu added a comment - Hi Akira Ajisaka and Chen Liang, Would you refer to the official link as follows? https://www.slf4j.org/faq.html#logging_performance Better yet, use parameterized messages There exists a very convenient alternative based on message formats. Assuming entry is an object, you can write: Object entry = new SomeObject(); logger.debug("The entry is {}.", entry); After evaluating whether to log or not, and only if the decision is affirmative, will the logger implementation format the message and replace the '{}' pair with the string value of entry. In other words, this form does not incur the cost of parameter construction in case the log statement is disabled. The following two lines will yield the exact same output. However, the second form will outperform the first form by a factor of at least 30, in case of a disabled logging statement. logger.debug("The new entry is " entry "."); logger.debug("The new entry is {}.", entry); And this is the source code: public void debug(String format, Object arg) { if(this.logger.isDebugEnabled()) { FormattingTuple ft = MessageFormatter.format(format, arg); this.logger.log(FQCN, Level.DEBUG, ft.getMessage(), ft.getThrowable()); } } public static final FormattingTuple format(String messagePattern, Object arg) { return arrayFormat(messagePattern, new Object[] {arg} ); } So, Arrays.asList(blocks) will not new the object until the format function. Looking forward to your next discussion.
        Hide
        ajisakaa Akira Ajisaka added a comment -

        Thanks Hui Xu, I think this is not right. In the following code, Arrays.asList(blocks) is called to evaluate the variable before calling LOG.debug().

        LOG.debug("blocks = {}", java.util.Arrays.asList(blocks));
        

        However, the second form will outperform the first form by a factor of at least 30, in case of a disabled logging statement.

        The second form skips Object.toString(), so it is faster. However, the form cannot skip the method call in the variable.

        Show
        ajisakaa Akira Ajisaka added a comment - Thanks Hui Xu , I think this is not right. In the following code, Arrays.asList(blocks) is called to evaluate the variable before calling LOG.debug() . LOG.debug( "blocks = {}" , java.util.Arrays.asList(blocks)); However, the second form will outperform the first form by a factor of at least 30, in case of a disabled logging statement. The second form skips Object.toString() , so it is faster. However, the form cannot skip the method call in the variable.
        Hide
        10075197 Hui Xu added a comment -

        Aha, I think you are correct.
        And in three or more arguments case, e.g:
        logger.debug("Value {} was inserted between {} and {}.", newVal, below, above);
        This form incurs the hidden cost of construction of an Object[] (object array) which is usually very small. The one and two argument variants do not incur this hidden cost and exist solely for this reason (efficiency).

        So, I have no more comment for the v004 patch by Chen Liang.
        Thank both of you again!

        Show
        10075197 Hui Xu added a comment - Aha, I think you are correct. And in three or more arguments case, e.g: logger.debug("Value {} was inserted between {} and {}.", newVal, below, above); This form incurs the hidden cost of construction of an Object[] (object array) which is usually very small. The one and two argument variants do not incur this hidden cost and exist solely for this reason (efficiency). So, I have no more comment for the v004 patch by Chen Liang. Thank both of you again!
        Hide
        vagarychen Chen Liang added a comment -

        Thanks Hui Xu for the followup. But just similar to Akira Ajisaka mentioned, I think the variable gets evaluated regardless of the log level. There is this discussion.

        Show
        vagarychen Chen Liang added a comment - Thanks Hui Xu for the followup. But just similar to Akira Ajisaka mentioned, I think the variable gets evaluated regardless of the log level. There is this discussion.
        Hide
        vagarychen Chen Liang added a comment -

        Seems our comments interleaved, did not see your comments when I was writing the one above

        Show
        vagarychen Chen Liang added a comment - Seems our comments interleaved, did not see your comments when I was writing the one above
        Hide
        10075197 Hui Xu added a comment -

        Hi,
        I want to know what to do next? Just waiting some member to merge the patch to 3.0.0-alpha3?
        Thanks!

        Show
        10075197 Hui Xu added a comment - Hi, I want to know what to do next? Just waiting some member to merge the patch to 3.0.0-alpha3? Thanks!
        Hide
        ajisakaa Akira Ajisaka added a comment -

        The patch mostly looks good to me. I'm thinking we can remove the guards from the following code because the methods just return fields.

                LOG.debug("Reported block {} on {} size {} replicaState = {}",
                    replica, dn, replica.getNumBytes(), reportedState);
        
              LOG.debug("Queueing reported block {} in state {}" +
                      " from datanode {} for later processing because {}.",
                  block, reportedState, storageInfo.getDatanodeDescriptor(), reason);
        
              LOG.debug("Reported block {} on {} size {} replicaState = {}",
                  block, node, block.getNumBytes(), reportedState);
        
                BlockManager.LOG
                    .debug("Block deletion is delayed during NameNode startup. "
                        + "The deletion will start after {} ms.", delay);
        
        Show
        ajisakaa Akira Ajisaka added a comment - The patch mostly looks good to me. I'm thinking we can remove the guards from the following code because the methods just return fields. LOG.debug( "Reported block {} on {} size {} replicaState = {}" , replica, dn, replica.getNumBytes(), reportedState); LOG.debug( "Queueing reported block {} in state {}" + " from datanode {} for later processing because {}." , block, reportedState, storageInfo.getDatanodeDescriptor(), reason); LOG.debug( "Reported block {} on {} size {} replicaState = {}" , block, node, block.getNumBytes(), reportedState); BlockManager.LOG .debug( "Block deletion is delayed during NameNode startup. " + "The deletion will start after {} ms." , delay);
        Hide
        10075197 Hui Xu added a comment -

        Got it. I've done the modification according to the latest suggestion from Akira Ajisaka. Would you review it again. Thanks!

        Show
        10075197 Hui Xu added a comment - Got it. I've done the modification according to the latest suggestion from Akira Ajisaka. Would you review it again. Thanks!
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 24s 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 14m 43s trunk passed
        +1 compile 0m 58s trunk passed
        +1 checkstyle 0m 42s trunk passed
        +1 mvnsite 0m 58s trunk passed
        +1 mvneclipse 0m 14s trunk passed
        +1 findbugs 1m 52s trunk passed
        +1 javadoc 0m 39s trunk passed
        +1 mvninstall 0m 58s the patch passed
        +1 compile 0m 46s the patch passed
        +1 javac 0m 46s the patch passed
        +1 checkstyle 0m 34s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 119 unchanged - 2 fixed = 119 total (was 121)
        +1 mvnsite 0m 52s the patch passed
        +1 mvneclipse 0m 11s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 53s the patch passed
        +1 javadoc 0m 37s the patch passed
        -1 unit 108m 5s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 21s The patch does not generate ASF License warnings.
        136m 5s



        Reason Tests
        Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting
          hadoop.hdfs.TestDFSStripedOutputStreamWithFailure080
          hadoop.hdfs.TestDFSStripedOutputStreamWithFailureWithRandomECPolicy



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:14b5c93
        JIRA Issue HDFS-11832
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12869819/HDFS-11832.005.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 5fbcc820692f 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / bc28da6
        Default Java 1.8.0_131
        findbugs v3.1.0-RC1
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/19610/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19610/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19610/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 24s 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 14m 43s trunk passed +1 compile 0m 58s trunk passed +1 checkstyle 0m 42s trunk passed +1 mvnsite 0m 58s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 52s trunk passed +1 javadoc 0m 39s trunk passed +1 mvninstall 0m 58s the patch passed +1 compile 0m 46s the patch passed +1 javac 0m 46s the patch passed +1 checkstyle 0m 34s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 119 unchanged - 2 fixed = 119 total (was 121) +1 mvnsite 0m 52s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 53s the patch passed +1 javadoc 0m 37s the patch passed -1 unit 108m 5s hadoop-hdfs in the patch failed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 136m 5s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure080   hadoop.hdfs.TestDFSStripedOutputStreamWithFailureWithRandomECPolicy Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HDFS-11832 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12869819/HDFS-11832.005.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 5fbcc820692f 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / bc28da6 Default Java 1.8.0_131 findbugs v3.1.0-RC1 unit https://builds.apache.org/job/PreCommit-HDFS-Build/19610/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/19610/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/19610/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        10075197 Hui Xu added a comment -

        Hi Akira Ajisaka,
        Would you please review the v005 patch again? It's based on your suggestion.
        Thanks!

        Show
        10075197 Hui Xu added a comment - Hi Akira Ajisaka, Would you please review the v005 patch again? It's based on your suggestion. Thanks!
        Hide
        ajisakaa Akira Ajisaka added a comment -

        +1, checking this in.

        Show
        ajisakaa Akira Ajisaka added a comment - +1, checking this in.
        Hide
        ajisakaa Akira Ajisaka added a comment -

        Committed this to trunk. Thanks Chen Liang and Hui Xu for the contribution!

        Show
        ajisakaa Akira Ajisaka added a comment - Committed this to trunk. Thanks Chen Liang and Hui Xu for the contribution!
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11795 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11795/)
        HDFS-11832. Switch leftover logs to slf4j format in BlockManager.java. (aajisaka: rev a7f085d6bf499edf23e650a4f7211c53a442da0e)

        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/InvalidateBlocks.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11795 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11795/ ) HDFS-11832 . Switch leftover logs to slf4j format in BlockManager.java. (aajisaka: rev a7f085d6bf499edf23e650a4f7211c53a442da0e) (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/InvalidateBlocks.java

          People

          • Assignee:
            vagarychen Chen Liang
            Reporter:
            10075197 Hui Xu
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development