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

Logging improvements in FSImageFormatProtobuf.Saver

    Details

    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      There are two missing LOG messages in FSImageFormat.Saver that are missing in FSImageFormatProtobuf.Saver, which mark start and end of fsimage saving. Would be good to have them logged for protobuf images as well.

      1. HDFS-10264.001.patch
        3 kB
        Xiaobing Zhou
      2. HDFS-10264.000.patch
        2 kB
        Xiaobing Zhou

        Activity

        Hide
        shv Konstantin Shvachko added a comment -

        Add the following to FSImageFormatProtobuf.Saver.save()

        1. LOG.info("Saving image file " + newFile + " using " + compression);
        2. LOG.info("Image file " + newFile + " of size " + newFile.length() +
                    " bytes saved in " + (now() - startTime)/1000 + " seconds.");
        Show
        shv Konstantin Shvachko added a comment - Add the following to FSImageFormatProtobuf.Saver.save() LOG.info("Saving image file " + newFile + " using " + compression); LOG.info( "Image file " + newFile + " of size " + newFile.length() + " bytes saved in " + (now() - startTime)/1000 + " seconds." );
        Hide
        boky01 Andras Bokor added a comment -

        Konstantin Shvachko
        Since the migrating from Log4j/Commons Logging to SLF4J is in progress gradually I suggest to use the following format:

            LOG.info("Saving image file {} using {}.", newFile, compression);
        
            LOG.info("Image file {} of size {} bytes saved in {} seconds.", newFile, newFile.length(), (now() - startTime)/1000);
        

        Also, the type of LOG variable needs to be changed.

        What do you think?

        Show
        boky01 Andras Bokor added a comment - Konstantin Shvachko Since the migrating from Log4j/Commons Logging to SLF4J is in progress gradually I suggest to use the following format: LOG.info( "Saving image file {} using {}." , newFile, compression); LOG.info( "Image file {} of size {} bytes saved in {} seconds." , newFile, newFile.length(), (now() - startTime)/1000); Also, the type of LOG variable needs to be changed. What do you think?
        Hide
        shv Konstantin Shvachko added a comment -

        Makes sense. Good suggestions, Andras.

        Show
        shv Konstantin Shvachko added a comment - Makes sense. Good suggestions, Andras.
        Hide
        xiaobingo Xiaobing Zhou added a comment -

        I posted the patch v000, please kindly review, thanks.

        Show
        xiaobingo Xiaobing Zhou added a comment - I posted the patch v000, please kindly review, thanks.
        Hide
        boky01 Andras Bokor added a comment -

        Xiaobing Zhou Please check the comments above. SLF4J is preferred.

        Show
        boky01 Andras Bokor added a comment - Xiaobing Zhou Please check the comments above. SLF4J is preferred.
        Hide
        xiaobingo Xiaobing Zhou added a comment -

        Andras Bokor and Konstantin Shvachko thanks for the comments. v001 used SLF4J logging.

        Show
        xiaobingo Xiaobing Zhou added a comment - Andras Bokor and Konstantin Shvachko thanks for the comments. v001 used SLF4J logging.
        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 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 9m 30s trunk passed
        +1 compile 1m 11s trunk passed with JDK v1.8.0_77
        +1 compile 1m 1s trunk passed with JDK v1.7.0_95
        +1 checkstyle 0m 29s trunk passed
        +1 mvnsite 1m 15s trunk passed
        +1 mvneclipse 0m 19s trunk passed
        +1 findbugs 2m 31s trunk passed
        +1 javadoc 1m 44s trunk passed with JDK v1.8.0_77
        +1 javadoc 2m 51s trunk passed with JDK v1.7.0_95
        +1 mvninstall 1m 8s the patch passed
        +1 compile 1m 11s the patch passed with JDK v1.8.0_77
        +1 javac 1m 11s the patch passed
        +1 compile 1m 1s the patch passed with JDK v1.7.0_95
        +1 javac 1m 1s the patch passed
        +1 checkstyle 0m 26s the patch passed
        +1 mvnsite 1m 13s the patch passed
        +1 mvneclipse 0m 18s the patch passed
        +1 whitespace 0m 0s Patch has no whitespace issues.
        +1 findbugs 3m 5s the patch passed
        +1 javadoc 1m 44s the patch passed with JDK v1.8.0_77
        +1 javadoc 2m 53s the patch passed with JDK v1.7.0_95
        -1 unit 102m 34s hadoop-hdfs in the patch failed with JDK v1.8.0_77.
        -1 unit 76m 52s hadoop-hdfs in the patch failed with JDK v1.7.0_95.
        +1 asflicense 0m 41s Patch does not generate ASF License warnings.
        217m 56s



        Reason Tests
        JDK v1.8.0_77 Failed junit tests hadoop.hdfs.server.datanode.TestDirectoryScanner
          hadoop.hdfs.server.namenode.ha.TestRetryCacheWithHA
          hadoop.hdfs.web.TestWebHdfsTimeouts
          hadoop.hdfs.server.namenode.ha.TestEditLogTailer
          hadoop.hdfs.server.datanode.TestDataNodeUUID
          hadoop.hdfs.shortcircuit.TestShortCircuitLocalRead
          hadoop.hdfs.security.TestDelegationTokenForProxyUser
          hadoop.hdfs.TestFileAppend
          hadoop.hdfs.server.namenode.TestNamenodeRetryCache
          hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure
        JDK v1.7.0_95 Failed junit tests hadoop.hdfs.server.datanode.TestDirectoryScanner
          hadoop.hdfs.server.namenode.ha.TestRetryCacheWithHA
          hadoop.hdfs.server.datanode.TestDataNodeLifeline
          hadoop.hdfs.shortcircuit.TestShortCircuitLocalRead
          hadoop.hdfs.TestPersistBlocks
          hadoop.hdfs.server.namenode.TestNamenodeRetryCache
          hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl
        JDK v1.7.0_95 Timed out junit tests org.apache.hadoop.hdfs.TestParallelShortCircuitLegacyRead
          org.apache.hadoop.hdfs.TestAclsEndToEnd
          org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
          org.apache.hadoop.hdfs.TestFileAppend2



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:fbe3e86
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12799367/HDFS-10264.001.patch
        JIRA Issue HDFS-10264
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 96eb7302c961 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / cb3ca46
        Default Java 1.7.0_95
        Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_77 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/15191/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/15191/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
        unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/15191/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15191/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt
        JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/15191/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/15191/console
        Powered by Apache Yetus 0.2.0 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 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 9m 30s trunk passed +1 compile 1m 11s trunk passed with JDK v1.8.0_77 +1 compile 1m 1s trunk passed with JDK v1.7.0_95 +1 checkstyle 0m 29s trunk passed +1 mvnsite 1m 15s trunk passed +1 mvneclipse 0m 19s trunk passed +1 findbugs 2m 31s trunk passed +1 javadoc 1m 44s trunk passed with JDK v1.8.0_77 +1 javadoc 2m 51s trunk passed with JDK v1.7.0_95 +1 mvninstall 1m 8s the patch passed +1 compile 1m 11s the patch passed with JDK v1.8.0_77 +1 javac 1m 11s the patch passed +1 compile 1m 1s the patch passed with JDK v1.7.0_95 +1 javac 1m 1s the patch passed +1 checkstyle 0m 26s the patch passed +1 mvnsite 1m 13s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 3m 5s the patch passed +1 javadoc 1m 44s the patch passed with JDK v1.8.0_77 +1 javadoc 2m 53s the patch passed with JDK v1.7.0_95 -1 unit 102m 34s hadoop-hdfs in the patch failed with JDK v1.8.0_77. -1 unit 76m 52s hadoop-hdfs in the patch failed with JDK v1.7.0_95. +1 asflicense 0m 41s Patch does not generate ASF License warnings. 217m 56s Reason Tests JDK v1.8.0_77 Failed junit tests hadoop.hdfs.server.datanode.TestDirectoryScanner   hadoop.hdfs.server.namenode.ha.TestRetryCacheWithHA   hadoop.hdfs.web.TestWebHdfsTimeouts   hadoop.hdfs.server.namenode.ha.TestEditLogTailer   hadoop.hdfs.server.datanode.TestDataNodeUUID   hadoop.hdfs.shortcircuit.TestShortCircuitLocalRead   hadoop.hdfs.security.TestDelegationTokenForProxyUser   hadoop.hdfs.TestFileAppend   hadoop.hdfs.server.namenode.TestNamenodeRetryCache   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure JDK v1.7.0_95 Failed junit tests hadoop.hdfs.server.datanode.TestDirectoryScanner   hadoop.hdfs.server.namenode.ha.TestRetryCacheWithHA   hadoop.hdfs.server.datanode.TestDataNodeLifeline   hadoop.hdfs.shortcircuit.TestShortCircuitLocalRead   hadoop.hdfs.TestPersistBlocks   hadoop.hdfs.server.namenode.TestNamenodeRetryCache   hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl JDK v1.7.0_95 Timed out junit tests org.apache.hadoop.hdfs.TestParallelShortCircuitLegacyRead   org.apache.hadoop.hdfs.TestAclsEndToEnd   org.apache.hadoop.hdfs.TestDFSStorageStateRecovery   org.apache.hadoop.hdfs.TestFileAppend2 Subsystem Report/Notes Docker Image:yetus/hadoop:fbe3e86 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12799367/HDFS-10264.001.patch JIRA Issue HDFS-10264 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 96eb7302c961 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / cb3ca46 Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_77 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/15191/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/15191/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/15191/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_77.txt https://builds.apache.org/job/PreCommit-HDFS-Build/15191/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_95.txt JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/15191/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/15191/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
        Hide
        xiaobingo Xiaobing Zhou added a comment -

        I verified the test failures are not related to this patch. Could anyone help to commit it? Thanks.

        Show
        xiaobingo Xiaobing Zhou added a comment - I verified the test failures are not related to this patch. Could anyone help to commit it? Thanks.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        +1 I will commit this shortly.

        Show
        arpitagarwal Arpit Agarwal added a comment - +1 I will commit this shortly.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        I've committed this. Thank you for the contribution Xiaobing Zhou.

        Show
        arpitagarwal Arpit Agarwal added a comment - I've committed this. Thank you for the contribution Xiaobing Zhou .
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #9634 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9634/)
        HDFS-10264. Logging improvements in FSImageFormatProtobuf.Saver. (arp: rev af9bdbe447b119bff10ec5281993bfc36b6dea71)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormatProtobuf.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #9634 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9634/ ) HDFS-10264 . Logging improvements in FSImageFormatProtobuf.Saver. (arp: rev af9bdbe447b119bff10ec5281993bfc36b6dea71) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormatProtobuf.java
        Hide
        shv Konstantin Shvachko added a comment -

        +1 from me too.
        Could we please commit this to 2.6 branch as well.

        Show
        shv Konstantin Shvachko added a comment - +1 from me too. Could we please commit this to 2.6 branch as well.
        Hide
        shv Konstantin Shvachko added a comment -

        Hey Arpit Agarwal, it looks like you committed this only to trunk, but the Fix Version says 2.7.3?

        Show
        shv Konstantin Shvachko added a comment - Hey Arpit Agarwal , it looks like you committed this only to trunk, but the Fix Version says 2.7.3?
        Show
        arpitagarwal Arpit Agarwal added a comment - That's odd. Are you sure your repo is synced? branch-2: https://git-wip-us.apache.org/repos/asf?p=hadoop.git;a=commit;h=b776db36f73b73097af52f98519b38aef8cc537d branch-2.7: https://git-wip-us.apache.org/repos/asf?p=hadoop.git;a=commit;h=12c6c2c9a6e929b26b6fa1e0acdac24205c922f3 branch-2.8: https://git-wip-us.apache.org/repos/asf?p=hadoop.git;a=commit;h=a13628fe4a7ee0bc1d803cfc983cf30a6f6cb665
        Hide
        shv Konstantin Shvachko added a comment -

        Sorry, please ignore the above: it is in 2.7.3. Merging into 2.6 now.

        Show
        shv Konstantin Shvachko added a comment - Sorry, please ignore the above: it is in 2.7.3. Merging into 2.6 now.
        Hide
        shv Konstantin Shvachko added a comment -

        My bad.

        Show
        shv Konstantin Shvachko added a comment - My bad.
        Hide
        shv Konstantin Shvachko added a comment -

        Committed to branch 2.6.

        Show
        shv Konstantin Shvachko added a comment - Committed to branch 2.6.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        Thanks for the backport Konstantin Shvachko. I added back 2.7.3 to 'Fix Version/s' as these parallel release lines (no semantic versioning).

        Show
        arpitagarwal Arpit Agarwal added a comment - Thanks for the backport Konstantin Shvachko . I added back 2.7.3 to 'Fix Version/s' as these parallel release lines (no semantic versioning).
        Hide
        shv Konstantin Shvachko added a comment -

        No problem, Arpit. I thought that we list the oldest version where the fix went in the "Fix version" field, which automatically means that it was also committed to higher version numbers.

        Show
        shv Konstantin Shvachko added a comment - No problem, Arpit. I thought that we list the oldest version where the fix went in the "Fix version" field, which automatically means that it was also committed to higher version numbers.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        Looks like we are making an exception for the parallel release lines.

        Show
        arpitagarwal Arpit Agarwal added a comment - Looks like we are making an exception for the parallel release lines.
        Hide
        vinodkv Vinod Kumar Vavilapalli added a comment -

        Closing the JIRA as part of 2.7.3 release.

        Show
        vinodkv Vinod Kumar Vavilapalli added a comment - Closing the JIRA as part of 2.7.3 release.

          People

          • Assignee:
            xiaobingo Xiaobing Zhou
            Reporter:
            shv Konstantin Shvachko
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development