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

More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data

    Details

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

      Description

      Currently SimulatedFSDataset uses a single DEFAULT_DATABYTE to simulate all block content. This is not accurate because the return of this byte just means the read request has hit an arbitrary position in an arbitrary simulated block.

      This JIRA aims to improve it with a more accurate verification. When position p of a simulated block b is accessed, the returned byte is b's block ID plus p, moduled by the max value of a byte.

      1. HDFS-8117.000.patch
        11 kB
        Zhe Zhang
      2. HDFS-8117.001.patch
        12 kB
        Zhe Zhang
      3. HDFS-8117.002.patch
        13 kB
        Zhe Zhang
      4. HDFS-8117.003.patch
        14 kB
        Zhe Zhang
      5. HDFS-8117-branch2.patch
        14 kB
        Zhe Zhang

        Activity

        Hide
        zhz Zhe Zhang added a comment -

        Thanks Andrew for reviewing and taking care of the commit!

        Show
        zhz Zhe Zhang added a comment - Thanks Andrew for reviewing and taking care of the commit!
        Hide
        andrew.wang Andrew Wang added a comment -

        LGTM, committed the branch-2 patch. Thanks Zhe!

        Show
        andrew.wang Andrew Wang added a comment - LGTM, committed the branch-2 patch. Thanks Zhe!
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2113 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2113/)
        HDFS-8117. More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2113 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2113/ ) HDFS-8117 . More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #164 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/164/)
        HDFS-8117. More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #164 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/164/ ) HDFS-8117 . More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #154 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/154/)
        HDFS-8117. More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #154 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/154/ ) HDFS-8117 . More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk #897 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/897/)
        HDFS-8117. More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #897 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/897/ ) HDFS-8117 . More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2095 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2095/)
        HDFS-8117. More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2095 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2095/ ) HDFS-8117 . More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #163 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/163/)
        HDFS-8117. More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #163 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/163/ ) HDFS-8117 . More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hadoopqa Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12725021/HDFS-8117.003.patch
        against trunk revision 7fc50e2.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 6 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10263//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10263//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12725021/HDFS-8117.003.patch against trunk revision 7fc50e2. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 6 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10263//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10263//console This message is automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12725038/HDFS-8117-branch2.patch
        against trunk revision d60e221.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10265//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12725038/HDFS-8117-branch2.patch against trunk revision d60e221. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10265//console This message is automatically generated.
        Hide
        zhz Zhe Zhang added a comment -

        Thanks Andrew for taking care of the commit. Patch for branch-2 attached.

        Show
        zhz Zhe Zhang added a comment - Thanks Andrew for taking care of the commit. Patch for branch-2 attached.
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #7575 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7575/)
        HDFS-8117. More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #7575 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7575/ ) HDFS-8117 . More accurate verification in SimulatedFSDataset: replace DEFAULT_DATABYTE with patterned data. Contributed by Zhe Zhang. (wang: rev d60e22152ac098da103fd37fb81f8758e68d1efa) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestSimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/SimulatedFSDataset.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSmallBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestFileAppend.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        andrew.wang Andrew Wang added a comment -

        Committed to trunk. Zhe, do you mind doing a branch-2 patch too? Some conflicts, not all of which were minor.

        Show
        andrew.wang Andrew Wang added a comment - Committed to trunk. Zhe, do you mind doing a branch-2 patch too? Some conflicts, not all of which were minor.
        Hide
        zhz Zhe Zhang added a comment -

        Thanks for the reviews Andrew, Colin, and Kai!

        Show
        zhz Zhe Zhang added a comment - Thanks for the reviews Andrew, Colin, and Kai!
        Hide
        andrew.wang Andrew Wang added a comment -

        LGTM +1 will commit shortly

        Show
        andrew.wang Andrew Wang added a comment - LGTM +1 will commit shortly
        Hide
        zhz Zhe Zhang added a comment -

        Thanks Andrew for the review!

        The new patch moves the util method of filling expected buffer to DFSTestUtil and now the logic is on LocatedBlocks level.

        Show
        zhz Zhe Zhang added a comment - Thanks Andrew for the review! The new patch moves the util method of filling expected buffer to DFSTestUtil and now the logic is on LocatedBlocks level.
        Hide
        andrew.wang Andrew Wang added a comment -

        Couple comments:

        • I still see a raw 12 that's not a variable, though it's better now. Name it numBlocksPerFile?
        • I have the same comment as Kai above about the forloop. Maybe we add a helper function to DFSTestUtil that takes a LocatedBlocks? Will make usage in the tests more convenient for sure.

        Otherwise good though, +1 pending.

        Show
        andrew.wang Andrew Wang added a comment - Couple comments: I still see a raw 12 that's not a variable, though it's better now. Name it numBlocksPerFile ? I have the same comment as Kai above about the forloop. Maybe we add a helper function to DFSTestUtil that takes a LocatedBlocks? Will make usage in the tests more convenient for sure. Otherwise good though, +1 pending.
        Hide
        hadoopqa Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12724466/HDFS-8117.002.patch
        against trunk revision 0959b67.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 5 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10252//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10252//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724466/HDFS-8117.002.patch against trunk revision 0959b67. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 5 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10252//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10252//console This message is automatically generated.
        Hide
        zhz Zhe Zhang added a comment -

        TestDFSClientRetries is unrelated and passes locally. Let's wait for a Jenkins run for 002 patch.

        Show
        zhz Zhe Zhang added a comment - TestDFSClientRetries is unrelated and passes locally. Let's wait for a Jenkins run for 002 patch.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12724418/HDFS-8117.001.patch
        against trunk revision af9d4fe.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 5 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.TestDFSClientRetries

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10251//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10251//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724418/HDFS-8117.001.patch against trunk revision af9d4fe. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 5 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDFSClientRetries Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10251//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10251//console This message is automatically generated.
        Hide
        drankye Kai Zheng added a comment -

        Thanks Zhe for the update and clarifying. It looks good to me.

        Show
        drankye Kai Zheng added a comment - Thanks Zhe for the update and clarifying. It looks good to me.
        Hide
        zhz Zhe Zhang added a comment -

        Thanks Kai. The new patch consolidate the logic for a set of blocks, because I want to avoid taking care of offsetInBuffer with transitioning between blocks (it doesn't always forward by the block's size).

        We could save a little more code if the new static method deals with LocatedBlock. But it doesn't make sense for SimulatedFSDataset, which is a DataNode component, to understand LocatedBlock.

        Show
        zhz Zhe Zhang added a comment - Thanks Kai. The new patch consolidate the logic for a set of blocks, because I want to avoid taking care of offsetInBuffer with transitioning between blocks (it doesn't always forward by the block's size). We could save a little more code if the new static method deals with LocatedBlock . But it doesn't make sense for SimulatedFSDataset , which is a DataNode component, to understand LocatedBlock .
        Hide
        drankye Kai Zheng added a comment -

        I haven't decided to verify the entire block or only part of it

        Sorry to clarify, what I meant is, to wrap the mentioned block of codes in a method, maybe something like below:

        public static byte simulatedBytes(Block b, long offsetInBlk, long len, byte[] buffer, long offsetInBuffer)  {
              for (int i = 0; i < len; ++i) {
                   byte firstByte = (byte) (b.getBlockId() % Byte.MAX_VALUE);
                   buffer[offsetInBuffer + i] = (byte) ((firstByte + offsetInBlk) % Byte.MAX_VALUE);
              }
         }
        
        Show
        drankye Kai Zheng added a comment - I haven't decided to verify the entire block or only part of it Sorry to clarify, what I meant is, to wrap the mentioned block of codes in a method, maybe something like below: public static byte simulatedBytes(Block b, long offsetInBlk, long len, byte [] buffer, long offsetInBuffer) { for ( int i = 0; i < len; ++i) { byte firstByte = ( byte ) (b.getBlockId() % Byte .MAX_VALUE); buffer[offsetInBuffer + i] = ( byte ) ((firstByte + offsetInBlk) % Byte .MAX_VALUE); } }
        Hide
        hadoopqa Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12724342/HDFS-8117.000.patch
        against trunk revision 623fd46.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 5 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10239//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10239//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724342/HDFS-8117.000.patch against trunk revision 623fd46. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 5 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10239//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10239//console This message is automatically generated.
        Hide
        zhz Zhe Zhang added a comment -

        Thanks Colin and Kai for reviewing! The 001 patch addresses the "12 blocks" issue.

        Maybe we can introduce a new method in SimulatedFSDataset like simulatedBlockBytes to wrap the blocks.

        Nice idea. I will use this new new code in verifying striped I/O in erasure coding. I haven't decided to verify the entire block or only part of it. If it turns out we always verify entire blocks, I will make this change.

        Show
        zhz Zhe Zhang added a comment - Thanks Colin and Kai for reviewing! The 001 patch addresses the "12 blocks" issue. Maybe we can introduce a new method in SimulatedFSDataset like simulatedBlockBytes to wrap the blocks. Nice idea. I will use this new new code in verifying striped I/O in erasure coding. I haven't decided to verify the entire block or only part of it. If it turns out we always verify entire blocks, I will make this change.
        Hide
        drankye Kai Zheng added a comment -

        The idea sounds great! A quick look at the patch:
        1. The codes like below are repeated quite some times. Maybe we can introduce a new method in SimulatedFSDataset like simulatedBlockBytes to wrap the blocks.

        +        for (long blkPos = 0; blkPos < lb.getBlockSize(); blkPos++) {
        +          expected[bufPos++] = SimulatedFSDataset.
        +              simulatedByte(lb.getBlock().getLocalBlock(), blkPos);
        

        2. Like mentioned above by Colin P. McCabe, 12 and 12*blockSize would be good to have meaningful names. I guess 12 should be bounded in other constants?

        Show
        drankye Kai Zheng added a comment - The idea sounds great! A quick look at the patch: 1. The codes like below are repeated quite some times. Maybe we can introduce a new method in SimulatedFSDataset like simulatedBlockBytes to wrap the blocks. + for ( long blkPos = 0; blkPos < lb.getBlockSize(); blkPos++) { + expected[bufPos++] = SimulatedFSDataset. + simulatedByte(lb.getBlock().getLocalBlock(), blkPos); 2. Like mentioned above by Colin P. McCabe , 12 and 12*blockSize would be good to have meaningful names. I guess 12 should be bounded in other constants?
        Hide
        cmccabe Colin P. McCabe added a comment -

        Looks good, Zhe Zhang.

        137	      LocatedBlocks lbs = dfs.getClient().getLocatedBlocks(name.toString(),
        138	          0, 12 * blockSize);
        

        While we're refactoring, can we have a named constant for this, rather than just "12"?

        +1 once that's addressed.

        Show
        cmccabe Colin P. McCabe added a comment - Looks good, Zhe Zhang . 137 LocatedBlocks lbs = dfs.getClient().getLocatedBlocks(name.toString(), 138 0, 12 * blockSize); While we're refactoring, can we have a named constant for this, rather than just "12"? +1 once that's addressed.
        Hide
        zhz Zhe Zhang added a comment -

        I also had to change some unit tests to actually obtain the list of blocks of a file, in order to verify file content.

        Show
        zhz Zhe Zhang added a comment - I also had to change some unit tests to actually obtain the list of blocks of a file, in order to verify file content.

          People

          • Assignee:
            zhz Zhe Zhang
            Reporter:
            zhz Zhe Zhang
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development