Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0.0-alpha2
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This is a sub task of HDFS-7674. It calculates the amount of data that is read from local or remote to attend decoding work, and also the amount of data that is written to local or remote datanodes.

      1. HDFS-8411.010.patch
        18 kB
        Andrew Wang
      2. HDFS-8411-001.patch
        4 kB
        Li Bo
      3. HDFS-8411-002.patch
        4 kB
        Li Bo
      4. HDFS-8411-003.patch
        4 kB
        Li Bo
      5. HDFS-8411-004.patch
        6 kB
        SammiChen
      6. HDFS-8411-005.patch
        12 kB
        SammiChen
      7. HDFS-8411-006.patch
        14 kB
        SammiChen
      8. HDFS-8411-007.patch
        14 kB
        SammiChen
      9. HDFS-8411-008.patch
        14 kB
        SammiChen
      10. HDFS-8411-009.patch
        15 kB
        SammiChen
      11. HDFS-8411-011.patch
        21 kB
        SammiChen
      12. HDFS-8411-012.patch
        20 kB
        SammiChen
      13. HDFS-8411-013.patch
        20 kB
        SammiChen

        Issue Links

          Activity

          Hide
          Sammi SammiChen added a comment -

          Thanks Andrew Wang, Rakesh R and Kai Zheng for the reviewing !

          Show
          Sammi SammiChen added a comment - Thanks Andrew Wang , Rakesh R and Kai Zheng for the reviewing !
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10995 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10995/)
          HDFS-8411. Add bytes count metrics to datanode for ECWorker. Contributed (kai.zheng: rev 1f14f6d038aecad55a5398c6fa4137c9d2f44729)

          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedReconstructor.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedBlockReconstructor.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeErasureCodingMetrics.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedBlockReader.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedReader.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/metrics/DataNodeMetrics.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedBlockWriter.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedWriter.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/StripedFileTestUtil.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10995 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10995/ ) HDFS-8411 . Add bytes count metrics to datanode for ECWorker. Contributed (kai.zheng: rev 1f14f6d038aecad55a5398c6fa4137c9d2f44729) (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedReconstructor.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedBlockReconstructor.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeErasureCodingMetrics.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedBlockReader.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedReader.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/metrics/DataNodeMetrics.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedBlockWriter.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/erasurecode/StripedWriter.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/StripedFileTestUtil.java
          Hide
          drankye Kai Zheng added a comment -

          Committed to trunk. Thanks SammiChen, Andrew Wang and Rakesh R for the contribution and reviewing.

          Show
          drankye Kai Zheng added a comment - Committed to trunk. Thanks SammiChen , Andrew Wang and Rakesh R for the contribution and reviewing.
          Hide
          drankye Kai Zheng added a comment -

          The latest patch LGTM and +1. Will commit it shortly.

          Show
          drankye Kai Zheng added a comment - The latest patch LGTM and +1. Will commit it shortly.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 22s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 7m 50s trunk passed
          +1 compile 0m 46s trunk passed
          +1 checkstyle 0m 28s trunk passed
          +1 mvnsite 0m 55s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 42s trunk passed
          +1 javadoc 0m 40s trunk passed
          +1 mvninstall 0m 46s the patch passed
          +1 compile 0m 44s the patch passed
          +1 javac 0m 44s the patch passed
          -0 checkstyle 0m 25s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 74 unchanged - 0 fixed = 76 total (was 74)
          +1 mvnsite 0m 48s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 46s the patch passed
          +1 javadoc 0m 37s the patch passed
          -1 unit 96m 17s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          116m 4s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.namenode.TestNameNodeMetadataConsistency
            hadoop.hdfs.TestSecureEncryptionZoneWithKMS
            hadoop.hdfs.TestTrashWithSecureEncryptionZones
            hadoop.hdfs.server.datanode.TestDirectoryScanner



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-8411
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842958/HDFS-8411-013.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux a68f9261b0a1 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / b0b033e
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17847/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17847/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17847/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17847/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 22s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 7m 50s trunk passed +1 compile 0m 46s trunk passed +1 checkstyle 0m 28s trunk passed +1 mvnsite 0m 55s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 42s trunk passed +1 javadoc 0m 40s trunk passed +1 mvninstall 0m 46s the patch passed +1 compile 0m 44s the patch passed +1 javac 0m 44s the patch passed -0 checkstyle 0m 25s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 74 unchanged - 0 fixed = 76 total (was 74) +1 mvnsite 0m 48s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 46s the patch passed +1 javadoc 0m 37s the patch passed -1 unit 96m 17s hadoop-hdfs in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 116m 4s Reason Tests Failed junit tests hadoop.hdfs.server.namenode.TestNameNodeMetadataConsistency   hadoop.hdfs.TestSecureEncryptionZoneWithKMS   hadoop.hdfs.TestTrashWithSecureEncryptionZones   hadoop.hdfs.server.datanode.TestDirectoryScanner Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-8411 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842958/HDFS-8411-013.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a68f9261b0a1 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / b0b033e Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17847/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/17847/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17847/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17847/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Change some wording according to Kai's offline suggestion

          Show
          Sammi SammiChen added a comment - Change some wording according to Kai's offline suggestion
          Hide
          drankye Kai Zheng added a comment -

          Thanks Sammi for the update! It needs a rebase since HDFS-11368 was in.

          Show
          drankye Kai Zheng added a comment - Thanks Sammi for the update! It needs a rebase since HDFS-11368 was in.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 9s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 8m 16s trunk passed
          +1 compile 0m 52s trunk passed
          +1 checkstyle 0m 29s trunk passed
          +1 mvnsite 1m 0s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 54s trunk passed
          +1 javadoc 0m 39s trunk passed
          +1 mvninstall 0m 53s the patch passed
          +1 compile 0m 54s the patch passed
          +1 javac 0m 54s the patch passed
          -0 checkstyle 0m 27s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 74 unchanged - 0 fixed = 76 total (was 74)
          +1 mvnsite 1m 1s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 2m 1s the patch passed
          +1 javadoc 0m 37s the patch passed
          -1 unit 85m 41s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 25s The patch does not generate ASF License warnings.
          107m 7s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestTrashWithSecureEncryptionZones
            hadoop.hdfs.TestSecureEncryptionZoneWithKMS
            hadoop.hdfs.TestEncryptionZones
            hadoop.hdfs.TestFileAppend3
            hadoop.hdfs.server.datanode.TestLargeBlockReport



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-8411
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842778/HDFS-8411-012.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux be57f809764e 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 4c38f11
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17834/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17834/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17834/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17834/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 9s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 8m 16s trunk passed +1 compile 0m 52s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 1m 0s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 54s trunk passed +1 javadoc 0m 39s trunk passed +1 mvninstall 0m 53s the patch passed +1 compile 0m 54s the patch passed +1 javac 0m 54s the patch passed -0 checkstyle 0m 27s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 74 unchanged - 0 fixed = 76 total (was 74) +1 mvnsite 1m 1s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 1s the patch passed +1 javadoc 0m 37s the patch passed -1 unit 85m 41s hadoop-hdfs in the patch failed. +1 asflicense 0m 25s The patch does not generate ASF License warnings. 107m 7s Reason Tests Failed junit tests hadoop.hdfs.TestTrashWithSecureEncryptionZones   hadoop.hdfs.TestSecureEncryptionZoneWithKMS   hadoop.hdfs.TestEncryptionZones   hadoop.hdfs.TestFileAppend3   hadoop.hdfs.server.datanode.TestLargeBlockReport Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-8411 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842778/HDFS-8411-012.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux be57f809764e 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 4c38f11 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17834/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/17834/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17834/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17834/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Improve the test case

          Show
          Sammi SammiChen added a comment - Improve the test case
          Hide
          Sammi SammiChen added a comment -

          Thanks Kai for reviewing the patch! I have uploaded new patch to address 2,3 and 4. For 1, it seems the bytesRead doesn't need a cleanup.

          Show
          Sammi SammiChen added a comment - Thanks Kai for reviewing the patch! I have uploaded new patch to address 2,3 and 4. For 1, it seems the bytesRead doesn't need a cleanup.
          Hide
          drankye Kai Zheng added a comment -

          Thanks Andrew Wang for the ping! It's good for me to have a very close to the codes and see the very good work here.

          1. In StripedReader the new variable bytesRead needs to be cleared during the loop. We don't run into this because our tests are using file length smaller than a strip.

          2. The following tests would be good to merge to save some test time:

          testEcTasks
          testEcCodingTime
          testEcBytesFullBlock
          

          By the way, inherited from existing codes, not necessary to have the Ec prefix as they're in the context of TestDataNodeErasureCodingMetrics.

          3. In the following codes, blockSize could be cellSize instead, otherwise it's very confusing and hard to understand the logic (the asserts).

            public void testEcBytesPartialGroup2() throws Exception {
              final int fileLen = blockSize + blockSize / 10;
              doTestForPartialGroup("/testEcBytes", fileLen, 0);
              // Add all reconstruction bytes read/write from all data nodes
              long bytesRead = 0;
              long bytesWrite = 0;
              for (DataNode dn : cluster.getDataNodes()) {
                MetricsRecordBuilder rb = getMetrics(dn.getMetrics().name());
                bytesRead += getLongCounter("EcReconstructionBytesRead", rb);
                bytesWrite += getLongCounter("EcReconstructionBytesWritten", rb);
              }
          
              Assert.assertEquals("ecReconstructionBytesRead should be ",
                  blockSize + blockSize / 10, bytesRead);
              Assert.assertEquals("ecReconstructionBytesWritten should be ",
                  blockSize, bytesWrite);
            }
          

          4. I'm happy to see this new trick to count the metrics more reliably by looking at all datanodes:

              for (DataNode dn : cluster.getDataNodes()) {
                MetricsRecordBuilder rb = getMetrics(dn.getMetrics().name());
                bytesRead += getLongCounter("EcReconstructionBytesRead", rb);
                bytesWrite += getLongCounter("EcReconstructionBytesWritten", rb);
              }
          

          So we could get rid of the trick played in doTest by using an extra datanode. In fact we could refactor and get rid of doTest entirely, by reusing the codes of doTestForPartialGroup. We could refactor doTestForPartialGroup to make it suitable for tests of all cases in file length considering the boundaries of cell, block, group, and more than one group.

          I thought 1, 2 and 3 are good to be fixed here and 4 could be done in a follow on issue as it's not trivial. SammiChen could you proceed to help with these? Thank you!

          Show
          drankye Kai Zheng added a comment - Thanks Andrew Wang for the ping! It's good for me to have a very close to the codes and see the very good work here. 1. In StripedReader the new variable bytesRead needs to be cleared during the loop. We don't run into this because our tests are using file length smaller than a strip. 2. The following tests would be good to merge to save some test time: testEcTasks testEcCodingTime testEcBytesFullBlock By the way, inherited from existing codes, not necessary to have the Ec prefix as they're in the context of TestDataNodeErasureCodingMetrics . 3. In the following codes, blockSize could be cellSize instead, otherwise it's very confusing and hard to understand the logic (the asserts). public void testEcBytesPartialGroup2() throws Exception { final int fileLen = blockSize + blockSize / 10; doTestForPartialGroup( "/testEcBytes" , fileLen, 0); // Add all reconstruction bytes read/write from all data nodes long bytesRead = 0; long bytesWrite = 0; for (DataNode dn : cluster.getDataNodes()) { MetricsRecordBuilder rb = getMetrics(dn.getMetrics().name()); bytesRead += getLongCounter( "EcReconstructionBytesRead" , rb); bytesWrite += getLongCounter( "EcReconstructionBytesWritten" , rb); } Assert.assertEquals( "ecReconstructionBytesRead should be " , blockSize + blockSize / 10, bytesRead); Assert.assertEquals( "ecReconstructionBytesWritten should be " , blockSize, bytesWrite); } 4. I'm happy to see this new trick to count the metrics more reliably by looking at all datanodes: for (DataNode dn : cluster.getDataNodes()) { MetricsRecordBuilder rb = getMetrics(dn.getMetrics().name()); bytesRead += getLongCounter( "EcReconstructionBytesRead" , rb); bytesWrite += getLongCounter( "EcReconstructionBytesWritten" , rb); } So we could get rid of the trick played in doTest by using an extra datanode. In fact we could refactor and get rid of doTest entirely, by reusing the codes of doTestForPartialGroup . We could refactor doTestForPartialGroup to make it suitable for tests of all cases in file length considering the boundaries of cell, block, group, and more than one group. I thought 1, 2 and 3 are good to be fixed here and 4 could be done in a follow on issue as it's not trivial. SammiChen could you proceed to help with these? Thank you!
          Hide
          andrew.wang Andrew Wang added a comment -

          Cool, thanks for the review Sammi! Kai Zheng mind giving this a final approval?

          Show
          andrew.wang Andrew Wang added a comment - Cool, thanks for the review Sammi! Kai Zheng mind giving this a final approval?
          Hide
          Sammi SammiChen added a comment - - edited

          Andrew, thanks for provide the sample patch! It perfectly count the bytes read in read failure case. I'm OK with the patch.

          I also checked the failed test case. It's irrelevant.

          Show
          Sammi SammiChen added a comment - - edited Andrew, thanks for provide the sample patch! It perfectly count the bytes read in read failure case. I'm OK with the patch. I also checked the failed test case. It's irrelevant.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 22s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 7m 3s trunk passed
          +1 compile 0m 47s trunk passed
          +1 checkstyle 0m 27s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 42s trunk passed
          +1 javadoc 0m 42s trunk passed
          +1 mvninstall 0m 49s the patch passed
          +1 compile 0m 45s the patch passed
          +1 javac 0m 45s the patch passed
          -0 checkstyle 0m 26s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 74 unchanged - 0 fixed = 76 total (was 74)
          +1 mvnsite 0m 54s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 52s the patch passed
          +1 javadoc 0m 41s the patch passed
          -1 unit 94m 25s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          114m 1s



          Reason Tests
          Failed junit tests hadoop.hdfs.web.TestWebHdfsFileSystemContract



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-8411
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842256/HDFS-8411.010.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 9fe230a05a1e 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / ea2895f
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17790/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17790/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17790/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17790/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 22s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 7m 3s trunk passed +1 compile 0m 47s trunk passed +1 checkstyle 0m 27s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 42s trunk passed +1 javadoc 0m 42s trunk passed +1 mvninstall 0m 49s the patch passed +1 compile 0m 45s the patch passed +1 javac 0m 45s the patch passed -0 checkstyle 0m 26s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 74 unchanged - 0 fixed = 76 total (was 74) +1 mvnsite 0m 54s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 52s the patch passed +1 javadoc 0m 41s the patch passed -1 unit 94m 25s hadoop-hdfs in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 114m 1s Reason Tests Failed junit tests hadoop.hdfs.web.TestWebHdfsFileSystemContract Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-8411 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842256/HDFS-8411.010.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 9fe230a05a1e 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ea2895f Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17790/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/17790/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17790/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17790/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          Here's a patch which illustrates my idea. It adds local counters per StripedReconstructor, which are incremented in StripedBlockReader and StripedBlockWriter. StripedBlockReconstructor then adds them to the DN-level metrics. This avoids incrementing for file checksum ops.

          Sammi, what do you think?

          Show
          andrew.wang Andrew Wang added a comment - Here's a patch which illustrates my idea. It adds local counters per StripedReconstructor, which are incremented in StripedBlockReader and StripedBlockWriter. StripedBlockReconstructor then adds them to the DN-level metrics. This avoids incrementing for file checksum ops. Sammi, what do you think?
          Hide
          Sammi SammiChen added a comment -

          Hi Andrew Wang, thanks for reviewing the patch!

          •In StripedReader, bytesRead is incremented when the future is submitted. However, we don't know if the future has succeeded until later on. Seems like we should increment only for successful requests?

          I'm do aware of that the future may fails. My current implementation bases on
          1. If the reconstruction task succeeds finally, no matter how many future fails, the actual successful byte read account is the same.
          2. If the final reconstruction task fails, the actual byte read account is inaccurate in this case. Given that current future task doesn't carry how many data is read, I trade off a bit of accuracy with the complexity introduced by refactor the future task information. I assume that the possibility is very low. If that's not the case, I will refine the patch for a more accurate account.

          •Why don't we increment the metrics directly in StripedReader / StripedWriter rather than doing it in StripedBlockReconstructor?

          Because StripedReader/StripedWriter is not only used in reconstructor work, but also other tasks, such as striped block checksum recalculation, that should be excluded from account into the metrics.

          JIRA HDFS-11216 created for follow-on.

          Show
          Sammi SammiChen added a comment - Hi Andrew Wang , thanks for reviewing the patch! •In StripedReader, bytesRead is incremented when the future is submitted. However, we don't know if the future has succeeded until later on. Seems like we should increment only for successful requests? I'm do aware of that the future may fails. My current implementation bases on 1. If the reconstruction task succeeds finally, no matter how many future fails, the actual successful byte read account is the same. 2. If the final reconstruction task fails, the actual byte read account is inaccurate in this case. Given that current future task doesn't carry how many data is read, I trade off a bit of accuracy with the complexity introduced by refactor the future task information. I assume that the possibility is very low. If that's not the case, I will refine the patch for a more accurate account. •Why don't we increment the metrics directly in StripedReader / StripedWriter rather than doing it in StripedBlockReconstructor? Because StripedReader/StripedWriter is not only used in reconstructor work, but also other tasks, such as striped block checksum recalculation, that should be excluded from account into the metrics. JIRA HDFS-11216 created for follow-on.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for working on this Sammi and Rakesh for reviewing, I had a few additional comments/questions:

          • In StripedReader, bytesRead is incremented when the future is submitted. However, we don't know if the future has succeeded until later on. Seems like we should increment only for successful requests?
          • Why don't we increment the metrics directly in StripedReader / StripedWriter rather than doing it in StripedBlockReconstructor?
          • If we aren't going to handle remote vs. local reconstruction targets in this JIRA, could you file a new JIRA to track this as a follow-on? It's pretty important, especially since on the destination side, it gets lumped together with other OP_WRITE calls.
          Show
          andrew.wang Andrew Wang added a comment - Thanks for working on this Sammi and Rakesh for reviewing, I had a few additional comments/questions: In StripedReader, bytesRead is incremented when the future is submitted. However, we don't know if the future has succeeded until later on. Seems like we should increment only for successful requests? Why don't we increment the metrics directly in StripedReader / StripedWriter rather than doing it in StripedBlockReconstructor? If we aren't going to handle remote vs. local reconstruction targets in this JIRA, could you file a new JIRA to track this as a follow-on? It's pretty important, especially since on the destination side, it gets lumped together with other OP_WRITE calls.
          Hide
          rakeshr Rakesh R added a comment -

          Thank you SammiChen, +1, latest patch looks good to me.

          Andrew Wang, could you please pitch in and would be great to see your feedback. Thanks!

          Show
          rakeshr Rakesh R added a comment - Thank you SammiChen , +1, latest patch looks good to me. Andrew Wang , could you please pitch in and would be great to see your feedback. Thanks!
          Hide
          Sammi SammiChen added a comment -

          Double checked the failed test cases, they are irrelevant to this patch.

          Show
          Sammi SammiChen added a comment - Double checked the failed test cases, they are irrelevant to this 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 appears to include 2 new or modified test files.
          +1 mvninstall 8m 46s trunk passed
          +1 compile 0m 57s trunk passed
          +1 checkstyle 0m 30s trunk passed
          +1 mvnsite 1m 3s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          -1 findbugs 1m 48s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings.
          +1 javadoc 0m 40s trunk passed
          +1 mvninstall 0m 49s the patch passed
          +1 compile 0m 49s the patch passed
          +1 javac 0m 49s the patch passed
          -0 checkstyle 0m 25s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 74 unchanged - 0 fixed = 76 total (was 74)
          +1 mvnsite 0m 55s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 49s the patch passed
          +1 javadoc 0m 37s the patch passed
          -1 unit 87m 35s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 19s The patch does not generate ASF License warnings.
          109m 13s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.namenode.ha.TestEditLogTailer
            hadoop.hdfs.TestDFSStripedOutputStreamWithFailure130



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-8411
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841698/HDFS-8411-009.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 367df13e2ef2 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f885160
          Default Java 1.8.0_111
          findbugs v3.0.0
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17754/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17754/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17754/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17754/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17754/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 8m 46s trunk passed +1 compile 0m 57s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 1m 3s trunk passed +1 mvneclipse 0m 14s trunk passed -1 findbugs 1m 48s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings. +1 javadoc 0m 40s trunk passed +1 mvninstall 0m 49s the patch passed +1 compile 0m 49s the patch passed +1 javac 0m 49s the patch passed -0 checkstyle 0m 25s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 74 unchanged - 0 fixed = 76 total (was 74) +1 mvnsite 0m 55s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 49s the patch passed +1 javadoc 0m 37s the patch passed -1 unit 87m 35s hadoop-hdfs in the patch failed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 109m 13s Reason Tests Failed junit tests hadoop.hdfs.server.namenode.ha.TestEditLogTailer   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure130 Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-8411 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841698/HDFS-8411-009.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 367df13e2ef2 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f885160 Default Java 1.8.0_111 findbugs v3.0.0 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17754/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17754/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/17754/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17754/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17754/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment - - edited

          Thanks Rakesh! I uploaded a new patch, removed the unused import.

          Show
          Sammi SammiChen added a comment - - edited Thanks Rakesh! I uploaded a new patch, removed the unused import.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks SammiChen for the continuous effort. Its nearing completion, it would be good to fix the checkstyle warning too.

          ./hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/metrics/DataNodeMetrics.java:21:import static org.apache.hadoop.metrics2.lib.Interns.info;:15: Unused import - org.apache.hadoop.metrics2.lib.Interns.info.
          
          Show
          rakeshr Rakesh R added a comment - Thanks SammiChen for the continuous effort. Its nearing completion, it would be good to fix the checkstyle warning too. ./hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/metrics/DataNodeMetrics.java:21: import static org.apache.hadoop.metrics2.lib.Interns.info;:15: Unused import - org.apache.hadoop.metrics2.lib.Interns.info.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 8m 33s trunk passed
          +1 compile 0m 50s trunk passed
          +1 checkstyle 0m 27s trunk passed
          +1 mvnsite 0m 54s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          -1 findbugs 1m 47s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings.
          +1 javadoc 0m 40s trunk passed
          +1 mvninstall 0m 49s the patch passed
          +1 compile 0m 44s the patch passed
          +1 javac 0m 44s the patch passed
          -0 checkstyle 0m 24s hadoop-hdfs-project/hadoop-hdfs: The patch generated 3 new + 74 unchanged - 0 fixed = 77 total (was 74)
          +1 mvnsite 0m 53s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 54s the patch passed
          +1 javadoc 0m 38s the patch passed
          +1 unit 88m 37s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          109m 30s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-8411
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841460/HDFS-8411-008.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux b0e49fb9d589 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / c87b3a4
          Default Java 1.8.0_111
          findbugs v3.0.0
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17742/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17742/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17742/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17742/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 8m 33s trunk passed +1 compile 0m 50s trunk passed +1 checkstyle 0m 27s trunk passed +1 mvnsite 0m 54s trunk passed +1 mvneclipse 0m 14s trunk passed -1 findbugs 1m 47s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings. +1 javadoc 0m 40s trunk passed +1 mvninstall 0m 49s the patch passed +1 compile 0m 44s the patch passed +1 javac 0m 44s the patch passed -0 checkstyle 0m 24s hadoop-hdfs-project/hadoop-hdfs: The patch generated 3 new + 74 unchanged - 0 fixed = 77 total (was 74) +1 mvnsite 0m 53s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 54s the patch passed +1 javadoc 0m 38s the patch passed +1 unit 88m 37s hadoop-hdfs in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 109m 30s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-8411 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841460/HDFS-8411-008.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux b0e49fb9d589 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / c87b3a4 Default Java 1.8.0_111 findbugs v3.0.0 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17742/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17742/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17742/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17742/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Thanks Rakesh R, It's a good point. I will upload a new patch. Besides the ecReconstructionBytesRead and ecReconstructionBytesWritten, I will also change ecDecodingTimeNanos .

          Show
          Sammi SammiChen added a comment - Thanks Rakesh R , It's a good point. I will upload a new patch. Besides the ecReconstructionBytesRead and ecReconstructionBytesWritten , I will also change ecDecodingTimeNanos .
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 9s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 6m 51s trunk passed
          +1 compile 0m 44s trunk passed
          +1 checkstyle 0m 27s trunk passed
          +1 mvnsite 0m 55s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          -1 findbugs 1m 41s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings.
          +1 javadoc 0m 39s trunk passed
          +1 mvninstall 0m 49s the patch passed
          +1 compile 0m 43s the patch passed
          +1 javac 0m 43s the patch passed
          -0 checkstyle 0m 26s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 74 unchanged - 0 fixed = 76 total (was 74)
          +1 mvnsite 0m 52s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 50s the patch passed
          +1 javadoc 0m 35s the patch passed
          -1 unit 61m 20s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          79m 51s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.blockmanagement.TestReconstructStripedBlocksWithRackAwareness
            hadoop.hdfs.TestCrcCorruption



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-8411
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841400/HDFS-8411-007.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux e6a845c6bc78 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / c87b3a4
          Default Java 1.8.0_111
          findbugs v3.0.0
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17737/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17737/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17737/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17737/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17737/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 9s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 6m 51s trunk passed +1 compile 0m 44s trunk passed +1 checkstyle 0m 27s trunk passed +1 mvnsite 0m 55s trunk passed +1 mvneclipse 0m 12s trunk passed -1 findbugs 1m 41s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings. +1 javadoc 0m 39s trunk passed +1 mvninstall 0m 49s the patch passed +1 compile 0m 43s the patch passed +1 javac 0m 43s the patch passed -0 checkstyle 0m 26s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 74 unchanged - 0 fixed = 76 total (was 74) +1 mvnsite 0m 52s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 50s the patch passed +1 javadoc 0m 35s the patch passed -1 unit 61m 20s hadoop-hdfs in the patch failed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 79m 51s Reason Tests Failed junit tests hadoop.hdfs.server.blockmanagement.TestReconstructStripedBlocksWithRackAwareness   hadoop.hdfs.TestCrcCorruption Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-8411 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841400/HDFS-8411-007.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux e6a845c6bc78 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / c87b3a4 Default Java 1.8.0_111 findbugs v3.0.0 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17737/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17737/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/17737/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17737/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17737/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          rakeshr Rakesh R added a comment -

          Thank you SammiChen for the good work. Apart from the following comment, I'm +1 for the patch.

          Just a suggestion to make the code cleaner and readable. Instead of adding description separately, can we keep the metric description along with the object reference like below,

            @Metric("Bytes read by erasure coding worker")
            MutableCounterLong ecReconstructionBytesRead;
            @Metric("Bytes written by erasure coding worker")
            MutableCounterLong ecReconstructionBytesWritten;
          
          Show
          rakeshr Rakesh R added a comment - Thank you SammiChen for the good work. Apart from the following comment, I'm +1 for the patch. Just a suggestion to make the code cleaner and readable. Instead of adding description separately, can we keep the metric description along with the object reference like below, @Metric( "Bytes read by erasure coding worker" ) MutableCounterLong ecReconstructionBytesRead; @Metric( "Bytes written by erasure coding worker" ) MutableCounterLong ecReconstructionBytesWritten;
          Hide
          Sammi SammiChen added a comment -

          1 fixe 2 checkstyle issue
          2.firebug is irrelevent
          3. last failed unit test is irrelevent

          Show
          Sammi SammiChen added a comment - 1 fixe 2 checkstyle issue 2.firebug is irrelevent 3. last failed unit test is irrelevent
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 11s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 6m 45s trunk passed
          +1 compile 0m 45s trunk passed
          +1 checkstyle 0m 26s trunk passed
          +1 mvnsite 0m 53s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          -1 findbugs 1m 39s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings.
          +1 javadoc 0m 39s trunk passed
          +1 mvninstall 0m 44s the patch passed
          +1 compile 0m 43s the patch passed
          +1 javac 0m 43s the patch passed
          -0 checkstyle 0m 24s hadoop-hdfs-project/hadoop-hdfs: The patch generated 4 new + 74 unchanged - 0 fixed = 78 total (was 74)
          +1 mvnsite 0m 48s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 47s the patch passed
          +1 javadoc 0m 36s the patch passed
          -1 unit 68m 34s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          86m 48s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.datanode.TestFsDatasetCache



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-8411
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841274/HDFS-8411-006.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 67bcb02de538 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 1f7613b
          Default Java 1.8.0_111
          findbugs v3.0.0
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17729/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17729/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17729/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17729/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17729/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 11s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 6m 45s trunk passed +1 compile 0m 45s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 0m 53s trunk passed +1 mvneclipse 0m 12s trunk passed -1 findbugs 1m 39s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings. +1 javadoc 0m 39s trunk passed +1 mvninstall 0m 44s the patch passed +1 compile 0m 43s the patch passed +1 javac 0m 43s the patch passed -0 checkstyle 0m 24s hadoop-hdfs-project/hadoop-hdfs: The patch generated 4 new + 74 unchanged - 0 fixed = 78 total (was 74) +1 mvnsite 0m 48s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 47s the patch passed +1 javadoc 0m 36s the patch passed -1 unit 68m 34s hadoop-hdfs in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 86m 48s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestFsDatasetCache Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-8411 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841274/HDFS-8411-006.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 67bcb02de538 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 1f7613b Default Java 1.8.0_111 findbugs v3.0.0 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17729/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17729/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/17729/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17729/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17729/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Thanks Rakesh R very much for helping review the patch!

          1. Striped reading logic is used by ec worker as well as block group checksum logic. Could you please rephrase the java comment and description

          Very good point! the metrics are about ECWorker. checksum reconstruction should not be included. I will update the update to only count the bytes read in ECWorker case.

          item 2 & 3 will also be taken care in new patch.
          the fireBug warning is irrelevant.
          2 checkstyle warnings will be handled. The other 2 are about ecReconstructionBytesWritten and ecReconstructionBytesRead must be private, and have accessor methods. I prefer leave these two checkstyle warnning alone.

          Show
          Sammi SammiChen added a comment - Thanks Rakesh R very much for helping review the patch! 1. Striped reading logic is used by ec worker as well as block group checksum logic. Could you please rephrase the java comment and description Very good point! the metrics are about ECWorker. checksum reconstruction should not be included. I will update the update to only count the bytes read in ECWorker case. item 2 & 3 will also be taken care in new patch. the fireBug warning is irrelevant. 2 checkstyle warnings will be handled. The other 2 are about ecReconstructionBytesWritten and ecReconstructionBytesRead must be private, and have accessor methods. I prefer leave these two checkstyle warnning alone.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks SammiChen for the work. I've few comments,

          1. Striped reading logic is used by ec worker as well as block group checksum logic. Could you please rephrase the java comment and description
            // Bytes read by erasure coding worker. to // Bytes read by ec striped block reconstructor
            Also, just a suggestion instead of adding description separately, can we use below way of adding description.
            @Metric("Bytes read by erasure coding striped block reconstructor")
            MutableCounterLong ecReconstructionBytesRead;
            @Metric("Bytes written by erasure coding striped block reconstructor")
            MutableCounterLong ecReconstructionBytesWritten;
            
            +    ecReconstructionBytesRead = registry.newCounter(
            +        info("ecReconstructionBytesRead", "Bytes read by erasure coding " +
            +            "worker"), (long) 0);
            +    ecReconstructionBytesWritten = registry.newCounter(
            +        info("ecReconstructionBytesWritten", "Bytes written by erasure " +
            +            "coding worker"), (long) 0);
            
          2. How about avoiding code duplication in testEcCodingTime and testEcBytesFullBlock by creating a method like below and use it in both the tests.
              private MetricsRecordBuilder waitForReconstructionFinished(DataNode workerDn)
                  throws TimeoutException, InterruptedException {
                MetricsRecordBuilder rb = getMetrics(workerDn.getMetrics().name());
                // Ensure that reconstruction task is finished
                GenericTestUtils.waitFor(new Supplier<Boolean>() {
                  @Override
                  public Boolean get() {
                    long taskMetricValue = getLongCounter("EcReconstructionTasks", rb);
                    return (taskMetricValue > 0);
                  }
                }, 500, 60000);
                return rb;
              }
            
          3. Else part is not required as it is throwing exception when there is no successful write, right? Can be like below,
                  int nSuccess = stripedWriter.transferData2Targets();
                  if (nSuccess == 0) {
                     String error = "Transfer failed for all targets.";
                     throw new IOException(error);
                  }
            
                  // update bytes written metrics
                  getDatanode().getMetrics().
                        incrECReconstructionBytesWritten(nSuccess * toReconstructLen);
                   }
            
          4. Please take care findbug warnings and fix necessary checkstyle warnings.
          Show
          rakeshr Rakesh R added a comment - Thanks SammiChen for the work. I've few comments, Striped reading logic is used by ec worker as well as block group checksum logic. Could you please rephrase the java comment and description // Bytes read by erasure coding worker. to // Bytes read by ec striped block reconstructor Also, just a suggestion instead of adding description separately, can we use below way of adding description. @Metric( "Bytes read by erasure coding striped block reconstructor" ) MutableCounterLong ecReconstructionBytesRead; @Metric( "Bytes written by erasure coding striped block reconstructor" ) MutableCounterLong ecReconstructionBytesWritten; + ecReconstructionBytesRead = registry.newCounter( + info( "ecReconstructionBytesRead" , "Bytes read by erasure coding " + + "worker" ), ( long ) 0); + ecReconstructionBytesWritten = registry.newCounter( + info( "ecReconstructionBytesWritten" , "Bytes written by erasure " + + "coding worker" ), ( long ) 0); How about avoiding code duplication in testEcCodingTime and testEcBytesFullBlock by creating a method like below and use it in both the tests. private MetricsRecordBuilder waitForReconstructionFinished(DataNode workerDn) throws TimeoutException, InterruptedException { MetricsRecordBuilder rb = getMetrics(workerDn.getMetrics().name()); // Ensure that reconstruction task is finished GenericTestUtils.waitFor( new Supplier< Boolean >() { @Override public Boolean get() { long taskMetricValue = getLongCounter( "EcReconstructionTasks" , rb); return (taskMetricValue > 0); } }, 500, 60000); return rb; } Else part is not required as it is throwing exception when there is no successful write, right? Can be like below, int nSuccess = stripedWriter.transferData2Targets(); if (nSuccess == 0) { String error = "Transfer failed for all targets." ; throw new IOException(error); } // update bytes written metrics getDatanode().getMetrics(). incrECReconstructionBytesWritten(nSuccess * toReconstructLen); } Please take care findbug warnings and fix necessary checkstyle warnings.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 12s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          +1 mvninstall 7m 19s trunk passed
          +1 compile 0m 44s trunk passed
          +1 checkstyle 0m 27s trunk passed
          +1 mvnsite 0m 52s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          -1 findbugs 1m 39s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings.
          +1 javadoc 0m 40s trunk passed
          +1 mvninstall 0m 45s the patch passed
          +1 compile 0m 43s the patch passed
          +1 javac 0m 43s the patch passed
          -0 checkstyle 0m 23s hadoop-hdfs-project/hadoop-hdfs: The patch generated 4 new + 74 unchanged - 0 fixed = 78 total (was 74)
          +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 53s the patch passed
          +1 javadoc 0m 36s the patch passed
          +1 unit 65m 56s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 24s The patch does not generate ASF License warnings.
          85m 4s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-8411
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841020/HDFS-8411-005.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 9eab93a5f7d9 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 51e6c1c
          Default Java 1.8.0_111
          findbugs v3.0.0
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17710/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17710/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17710/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17710/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. +1 mvninstall 7m 19s trunk passed +1 compile 0m 44s trunk passed +1 checkstyle 0m 27s trunk passed +1 mvnsite 0m 52s trunk passed +1 mvneclipse 0m 13s trunk passed -1 findbugs 1m 39s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings. +1 javadoc 0m 40s trunk passed +1 mvninstall 0m 45s the patch passed +1 compile 0m 43s the patch passed +1 javac 0m 43s the patch passed -0 checkstyle 0m 23s hadoop-hdfs-project/hadoop-hdfs: The patch generated 4 new + 74 unchanged - 0 fixed = 78 total (was 74) +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 53s the patch passed +1 javadoc 0m 36s the patch passed +1 unit 65m 56s hadoop-hdfs in the patch passed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 85m 4s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-8411 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841020/HDFS-8411-005.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 9eab93a5f7d9 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 51e6c1c Default Java 1.8.0_111 findbugs v3.0.0 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17710/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17710/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17710/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17710/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          1. support partial read/write case
          2. add test case for partical read/write case

          Show
          Sammi SammiChen added a comment - 1. support partial read/write case 2. add test case for partical read/write case
          Hide
          Sammi SammiChen added a comment -

          Hi Andrew Wang, thanks for review the patch! I will upload a new patch to address the comments.

          It looks like the metrics only increment at the block granularity, it doesn't track partial block reads/writes that then fail.

          You're right. The next patch will track both block granularity and partial blcok reads/writes.

          To normalize with the other reconstruction metrics, maybe name "ecReconstructionBytesRead" and "ecReconstructionBytesWritten"

          handled.

          This is an optional comment (can do in follow-on maybe), but since it looks like the ECWorker can write both locally and remote, should we differentiate these as well? This is like how we differentiate remote vs. local reads in FileSystem$Statistics. e.g. bytesRead, bytesReadLocalHost, bytesReadDistanceOfOneOrTwo, etc.

          I would prefer it as a follow-on

          Show
          Sammi SammiChen added a comment - Hi Andrew Wang , thanks for review the patch! I will upload a new patch to address the comments. It looks like the metrics only increment at the block granularity, it doesn't track partial block reads/writes that then fail. You're right. The next patch will track both block granularity and partial blcok reads/writes. To normalize with the other reconstruction metrics, maybe name "ecReconstructionBytesRead" and "ecReconstructionBytesWritten" handled. This is an optional comment (can do in follow-on maybe), but since it looks like the ECWorker can write both locally and remote, should we differentiate these as well? This is like how we differentiate remote vs. local reads in FileSystem$Statistics. e.g. bytesRead, bytesReadLocalHost, bytesReadDistanceOfOneOrTwo, etc. I would prefer it as a follow-on
          Hide
          Sammi SammiChen added a comment -

          Hi Andrew, thanks for provide so many insightful comments. I will do some digging accordingly.

          Show
          Sammi SammiChen added a comment - Hi Andrew, thanks for provide so many insightful comments. I will do some digging accordingly.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for picking this up SammiChen, I'd like to go a bit deeper in this patch.

          • It looks like the metrics only increment at the block granularity, it doesn't track partial block reads/writes that then fail.
          • To normalize with the other reconstruction metrics, maybe name "ecReconstructionBytesRead" and "ecReconstructionBytesWritten"?
          • This is an optional comment (can do in follow-on maybe), but since it looks like the ECWorker can write both locally and remote, should we differentiate these as well? This is like how we differentiate remote vs. local reads in FileSystem$Statistics. e.g. bytesRead, bytesReadLocalHost, bytesReadDistanceOfOneOrTwo, etc.
          Show
          andrew.wang Andrew Wang added a comment - Thanks for picking this up SammiChen , I'd like to go a bit deeper in this patch. It looks like the metrics only increment at the block granularity, it doesn't track partial block reads/writes that then fail. To normalize with the other reconstruction metrics, maybe name "ecReconstructionBytesRead" and "ecReconstructionBytesWritten"? This is an optional comment (can do in follow-on maybe), but since it looks like the ECWorker can write both locally and remote, should we differentiate these as well? This is like how we differentiate remote vs. local reads in FileSystem$Statistics. e.g. bytesRead, bytesReadLocalHost, bytesReadDistanceOfOneOrTwo, etc.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 8m 59s trunk passed
          +1 compile 0m 49s trunk passed
          +1 checkstyle 0m 28s trunk passed
          +1 mvnsite 0m 55s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 48s trunk passed
          +1 javadoc 0m 41s trunk passed
          +1 mvninstall 0m 48s the patch passed
          +1 compile 0m 44s the patch passed
          +1 javac 0m 44s the patch passed
          -0 checkstyle 0m 25s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 64 unchanged - 0 fixed = 66 total (was 64)
          +1 mvnsite 0m 50s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 1s The patch has no whitespace issues.
          +1 findbugs 1m 52s the patch passed
          +1 javadoc 0m 37s the patch passed
          -1 unit 59m 41s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 28s The patch does not generate ASF License warnings.
          81m 6s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.namenode.TestFSEditLogLoader
            hadoop.hdfs.TestEncryptionZones



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-8411
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12834145/HDFS-8411-004.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 3ea0fbb3dd73 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 / c5573e6
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17216/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17216/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17216/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17216/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 8m 59s trunk passed +1 compile 0m 49s trunk passed +1 checkstyle 0m 28s trunk passed +1 mvnsite 0m 55s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 48s trunk passed +1 javadoc 0m 41s trunk passed +1 mvninstall 0m 48s the patch passed +1 compile 0m 44s the patch passed +1 javac 0m 44s the patch passed -0 checkstyle 0m 25s hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 64 unchanged - 0 fixed = 66 total (was 64) +1 mvnsite 0m 50s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 1s The patch has no whitespace issues. +1 findbugs 1m 52s the patch passed +1 javadoc 0m 37s the patch passed -1 unit 59m 41s hadoop-hdfs in the patch failed. +1 asflicense 0m 28s The patch does not generate ASF License warnings. 81m 6s Reason Tests Failed junit tests hadoop.hdfs.server.namenode.TestFSEditLogLoader   hadoop.hdfs.TestEncryptionZones Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-8411 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12834145/HDFS-8411-004.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 3ea0fbb3dd73 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 / c5573e6 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17216/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/17216/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17216/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17216/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Rebase and refactor the patch

          Show
          Sammi SammiChen added a comment - Rebase and refactor the patch
          Hide
          andrew.wang Andrew Wang added a comment -

          I think generally, more metrics are useful. HDFS-8529 I just closed as a dupe since we have task-level metrics, but HDFS-8410 and this JIRA are nice since it indicates whether the bottleneck is I/O or CPU.

          I also like this bytes-read metric a lot. It'd be nice to have a similar metric for replication work if we don't, as well as a "total" metric that includes both.

          Show
          andrew.wang Andrew Wang added a comment - I think generally, more metrics are useful. HDFS-8529 I just closed as a dupe since we have task-level metrics, but HDFS-8410 and this JIRA are nice since it indicates whether the bottleneck is I/O or CPU. I also like this bytes-read metric a lot. It'd be nice to have a similar metric for replication work if we don't, as well as a "total" metric that includes both.
          Hide
          libo-intel Li Bo added a comment -

          When some datanodes are corrupted, all their blocks are to be reconstructed by other healthy datanodes. The network flow incurred is very high and maybe we want to track it. We can record the bytes read and written by any datanode. In fact, I think HDFS-8529(block counts) and HDFS-8410(time consumed) are not necessary. We can estimate the time cost according to the bytes read and write. Block count metric is not very meaningful when there’re a lot of small files. We can adjust the metrics for the future requirement.

          Show
          libo-intel Li Bo added a comment - When some datanodes are corrupted, all their blocks are to be reconstructed by other healthy datanodes. The network flow incurred is very high and maybe we want to track it. We can record the bytes read and written by any datanode. In fact, I think HDFS-8529 (block counts) and HDFS-8410 (time consumed) are not necessary. We can estimate the time cost according to the bytes read and write. Block count metric is not very meaningful when there’re a lot of small files. We can adjust the metrics for the future requirement.
          Hide
          walter.k.su Walter Su added a comment -

          I think this metric is unnecessary. Block replication has no bytes count either. You can keep this if you want. Btw, ecBytesRead can be easily mistaken for the bytes read by Client.

          Show
          walter.k.su Walter Su added a comment - I think this metric is unnecessary. Block replication has no bytes count either. You can keep this if you want. Btw, ecBytesRead can be easily mistaken for the bytes read by Client.

            People

            • Assignee:
              Sammi SammiChen
              Reporter:
              libo-intel Li Bo
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development