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

Split getAdditionalBlock() into two methods.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.7.0
    • Fix Version/s: 2.8.0, 2.7.1, 3.0.0-alpha1
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      A minor refactoring to introduce two methods one corresponding to Part I and another to Part II to make getAdditionalBlock() more readable.

      1. HDFS-8081-01.patch
        4 kB
        Konstantin Shvachko
      2. HDFS-8081-02.patch
        4 kB
        Konstantin Shvachko
      3. HDFS-8081-03.patch
        10 kB
        Konstantin Shvachko

        Activity

        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2109 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2109/)
        HDFS-8081. Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2109 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2109/ ) HDFS-8081 . Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #160 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/160/)
        HDFS-8081. Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #160 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/160/ ) HDFS-8081 . Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #150 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/150/)
        HDFS-8081. Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #150 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/150/ ) HDFS-8081 . Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2091 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2091/)
        HDFS-8081. Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2091 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2091/ ) HDFS-8081 . Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #159 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/159/)
        HDFS-8081. Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #159 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/159/ ) HDFS-8081 . Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk #893 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/893/)
        HDFS-8081. Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #893 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/893/ ) HDFS-8081 . Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        Hide
        shv Konstantin Shvachko added a comment -

        I just committed this.
        Pushed to branch-2.7 as minor server-side improvement.

        Show
        shv Konstantin Shvachko added a comment - I just committed this. Pushed to branch-2.7 as minor server-side improvement.
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #7556 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7556/)
        HDFS-8081. Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #7556 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7556/ ) HDFS-8081 . Split getAdditionalBlock() into two methods. Contributed by Konstantin Shvachko (shv: rev 0959b67f1a189b4a99752904115efbd471f1d6d7) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hitliuyi Yi Liu added a comment -
        Show
        hitliuyi Yi Liu added a comment - +1, thanks Konstantin Shvachko
        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/12724326/HDFS-8081-03.patch
        against trunk revision a813db0.

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

        +1 tests included. The patch appears to include 1 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 test build failed in hadoop-hdfs-project/hadoop-hdfs

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10236//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10236//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/12724326/HDFS-8081-03.patch against trunk revision a813db0. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 test build failed in hadoop-hdfs-project/hadoop-hdfs Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10236//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10236//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/12724279/HDFS-8081-02.patch
        against trunk revision 63c659d.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. 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/10232//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10232//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/12724279/HDFS-8081-02.patch against trunk revision 63c659d. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . 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/10232//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10232//console This message is automatically generated.
        Hide
        shv Konstantin Shvachko added a comment -

        This actually allows to simplify TestAddBlockRetry - do not need to mock chooseTargets() any more. This version simplifies the test.

        Show
        shv Konstantin Shvachko added a comment - This actually allows to simplify TestAddBlockRetry - do not need to mock chooseTargets() any more. This version simplifies the test.
        Hide
        shv Konstantin Shvachko added a comment -

        Fixed the nits. Thanks Yi.

        Show
        shv Konstantin Shvachko added a comment - Fixed the nits. Thanks Yi.
        Hide
        hitliuyi Yi Liu added a comment -

        Hi Konst, the patch looks good. Few nits:
        1.

        // Part I. Analyze the state of the file with respect to the input data.

        // Part II.
        // Allocate a new block, add it to the INode and the BlocksMap.

        Since in the patch we have two methods, could we remove the original comments since the new methods comment already cover them.

        2.

        if (targets == null && onRetryBlock[0] != null) {
        

        For current code logic, if targest is null, then onRetryBlock[0] != null should be true (Since chooseTarget4NewBlock will never return null).
        Could you remove onRetryBlock[0] != null or just assert it? Otherwise it has some misunderstanding: even targets == null, if onRetryBlock[0] is null, then we still call storeAllocatedBlock.

        Show
        hitliuyi Yi Liu added a comment - Hi Konst, the patch looks good. Few nits: 1. // Part I. Analyze the state of the file with respect to the input data. // Part II. // Allocate a new block, add it to the INode and the BlocksMap. Since in the patch we have two methods, could we remove the original comments since the new methods comment already cover them. 2. if (targets == null && onRetryBlock[0] != null ) { For current code logic, if targest is null, then onRetryBlock[0] != null should be true (Since chooseTarget4NewBlock will never return null). Could you remove onRetryBlock[0] != null or just assert it? Otherwise it has some misunderstanding: even targets == null , if onRetryBlock[0] is null, then we still call storeAllocatedBlock .
        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/12723798/HDFS-8081-01.patch
        against trunk revision 5b8a3ae.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. 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/10202//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10202//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/12723798/HDFS-8081-01.patch against trunk revision 5b8a3ae. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . 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/10202//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10202//console This message is automatically generated.
        Hide
        shv Konstantin Shvachko added a comment -

        This is a rather straightforward split of getAdditionalBlock() into sub-methods.

        Show
        shv Konstantin Shvachko added a comment - This is a rather straightforward split of getAdditionalBlock() into sub-methods.

          People

          • Assignee:
            shv Konstantin Shvachko
            Reporter:
            shv Konstantin Shvachko
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development