Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      We can do some code cleanup for LocatedBlock, including:

      1. Using a simple Builder pattern to avoid multiple constructors
      2. Setting data fields like corrupt and offset to final
      1. HDFS-7829.1.patch
        3 kB
        Takanobu Asanuma
      2. HDFS-7829.2.patch
        35 kB
        Takanobu Asanuma
      3. HDFS-7829.3.patch
        38 kB
        Takanobu Asanuma
      4. HDFS-7829.4.patch
        8 kB
        Takanobu Asanuma

        Activity

        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        I'd like to try to do this issue. Please assign it to me, thank you.

        Show
        tasanuma0829 Takanobu Asanuma added a comment - I'd like to try to do this issue. Please assign it to me, thank you.
        Hide
        jingzhao Jing Zhao added a comment -

        Hi Takanobu Asanuma, I just assigned this to you. Thanks for working on this!

        Show
        jingzhao Jing Zhao added a comment - Hi Takanobu Asanuma , I just assigned this to you. Thanks for working on this!
        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        I implemented Builder pattern in LocatedBlock and submitted the patch. If there is no problem, I'd like to confirm the following next steps.

        1. Fixing all source codes calling constructor of LocatedBlock to use builder.
        2. Removing multiple constructors.
        3. Setting data fields to final.

        How does that look? Thank you.

        Show
        tasanuma0829 Takanobu Asanuma added a comment - I implemented Builder pattern in LocatedBlock and submitted the patch. If there is no problem, I'd like to confirm the following next steps. 1. Fixing all source codes calling constructor of LocatedBlock to use builder. 2. Removing multiple constructors. 3. Setting data fields to final. How does that look? Thank you.
        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/12702758/HDFS-7829.1.patch
        against trunk revision 5e9b814.

        +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/9743//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9743//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/12702758/HDFS-7829.1.patch against trunk revision 5e9b814. +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/9743//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9743//console This message is automatically generated.
        Hide
        jingzhao Jing Zhao added a comment -

        Thanks for the update, Takanobu Asanuma!

        The current builder implementation looks good to me in general. The plan also sounds good to me. Some comments on the builder:

        1. "this.cachedLocs" should be changed to "this.locs"?
          +    public Builder setStorages(DatanodeStorageInfo[] storages) {
          +      this.cachedLocs = DatanodeStorageInfo.toDatanodeInfos(storages);
          +      this.storageIDs = DatanodeStorageInfo.toStorageIDs(storages);
          +      this.storageTypes = DatanodeStorageInfo.toStorageTypes(storages);
          +      return this;
          +    }
          
        2. Looking into the current constructors, seems we're mainly handling two scenarios:
          1) The caller passes in a "DatanodeStorageInfo[]" which is used to set locs, storageIDs, and storageTypes.
          2) The caller passes in a "DatanodeInfo[]" which is used to set locs, while storageIDs and storageTypes are left as null.
          Thus it may be better to reflect this dependency in the builder. More specifically, I think we don't need setStrageIDs and setStorageTypes in the builder.
        Show
        jingzhao Jing Zhao added a comment - Thanks for the update, Takanobu Asanuma ! The current builder implementation looks good to me in general. The plan also sounds good to me. Some comments on the builder: "this.cachedLocs" should be changed to "this.locs"? + public Builder setStorages(DatanodeStorageInfo[] storages) { + this .cachedLocs = DatanodeStorageInfo.toDatanodeInfos(storages); + this .storageIDs = DatanodeStorageInfo.toStorageIDs(storages); + this .storageTypes = DatanodeStorageInfo.toStorageTypes(storages); + return this ; + } Looking into the current constructors, seems we're mainly handling two scenarios: 1) The caller passes in a "DatanodeStorageInfo[]" which is used to set locs , storageIDs , and storageTypes . 2) The caller passes in a "DatanodeInfo[]" which is used to set locs , while storageIDs and storageTypes are left as null. Thus it may be better to reflect this dependency in the builder. More specifically, I think we don't need setStrageIDs and setStorageTypes in the builder.
        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        Thank you for your kind review, Jing! I reply your comments:

        1. This is my mistake. I will correct it, thank you!

        2. I think there is the third scenario.
        3) The caller set "DatanodeInfo[]", storageIDs and strorageTypes.
        It is in ReportBadBlockAction.reportTo and TestDatanodeManager.testSortLocatedBlocks. So I think we can't remove setStrageIDs and setStorageTypes from the builder. How about this?

        Show
        tasanuma0829 Takanobu Asanuma added a comment - Thank you for your kind review, Jing! I reply your comments: 1. This is my mistake. I will correct it, thank you! 2. I think there is the third scenario. 3) The caller set "DatanodeInfo[]", storageIDs and strorageTypes . It is in ReportBadBlockAction.reportTo and TestDatanodeManager.testSortLocatedBlocks . So I think we can't remove setStrageIDs and setStorageTypes from the builder. How about this?
        Hide
        jingzhao Jing Zhao added a comment -

        Ahh, you're right. We have the third scenario here. Thanks Takanobu!

        Show
        jingzhao Jing Zhao added a comment - Ahh, you're right. We have the third scenario here. Thanks Takanobu!
        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        I submitted a patch again.

        I found that RecoveringBlock inherited LocatedBlock. So I also implemented Builder pattern in RecoveringBlock and fixed codes in LocatedBlock to use generics. I referred to http://stackoverflow.com/questions/17164375/subclassing-a-java-builder-class. This may be little complicated. And I fixed all codes calling constructors of these classes.

        Please would you review my patch again. If it is no problem, I will remove multiple constructors in LocatedBlock and RecoveringBlock. Thank you.

        Show
        tasanuma0829 Takanobu Asanuma added a comment - I submitted a patch again. I found that RecoveringBlock inherited LocatedBlock. So I also implemented Builder pattern in RecoveringBlock and fixed codes in LocatedBlock to use generics. I referred to http://stackoverflow.com/questions/17164375/subclassing-a-java-builder-class . This may be little complicated. And I fixed all codes calling constructors of these classes. Please would you review my patch again. If it is no problem, I will remove multiple constructors in LocatedBlock and RecoveringBlock. Thank you.
        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/12703404/HDFS-7829.2.patch
        against trunk revision 5578e22.

        -1 patch. Trunk compilation may be broken.

        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9791//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/12703404/HDFS-7829.2.patch against trunk revision 5578e22. -1 patch . Trunk compilation may be broken. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9791//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/12703409/HDFS-7829.2.patch
        against trunk revision 5578e22.

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

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

        -1 javac. The applied patch generated 1162 javac compiler warnings (more than the trunk's current 1155 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.server.namenode.ha.TestRetryCacheWithHA

        The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.server.blockmanagement.TestDatanodeManager

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9793//testReport/
        Javac warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9793//artifact/patchprocess/diffJavacWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9793//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/12703409/HDFS-7829.2.patch against trunk revision 5578e22. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 8 new or modified test files. -1 javac . The applied patch generated 1162 javac compiler warnings (more than the trunk's current 1155 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.server.namenode.ha.TestRetryCacheWithHA The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.blockmanagement.TestDatanodeManager Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9793//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9793//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9793//console This message is automatically generated.
        Hide
        jingzhao Jing Zhao added a comment -

        Thanks Takanobu. The current patch looks good to me. My only concern is that maybe we do not need to use Builder for RecoveringBlock. But please go ahead with the full patch.

        Show
        jingzhao Jing Zhao added a comment - Thanks Takanobu. The current patch looks good to me. My only concern is that maybe we do not need to use Builder for RecoveringBlock. But please go ahead with the full patch.
        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        Thanks for your review, Jin. The constructor in RecoveringBlock calls super constructor with four parameters. So I implemented builder pattern in RecoveringBlock to remove all public constructor in LocatedBlock. But it may be better to remain constructor with four parameters in LocatedBlock. Although I implemented builder in RecoveringBlock, I will fix my patch if there is better idea.

        Show
        tasanuma0829 Takanobu Asanuma added a comment - Thanks for your review, Jin. The constructor in RecoveringBlock calls super constructor with four parameters. So I implemented builder pattern in RecoveringBlock to remove all public constructor in LocatedBlock. But it may be better to remain constructor with four parameters in LocatedBlock. Although I implemented builder in RecoveringBlock, I will fix my patch if there is better idea.
        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        I resubmitted my patch.

        I removed all public constructors and set variables to final as much as possible. Please review it again. Thank you.

        Show
        tasanuma0829 Takanobu Asanuma added a comment - I resubmitted my patch. I removed all public constructors and set variables to final as much as possible. Please review it again. Thank you.
        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/12703628/HDFS-7829.3.patch
        against trunk revision 7711049.

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

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

        -1 javac. The applied patch generated 1162 javac compiler warnings (more than the trunk's current 1155 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.server.blockmanagement.TestDatanodeManager
        org.apache.hadoop.hdfs.server.namenode.TestFileTruncate

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9815//testReport/
        Javac warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9815//artifact/patchprocess/diffJavacWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9815//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/12703628/HDFS-7829.3.patch against trunk revision 7711049. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 8 new or modified test files. -1 javac . The applied patch generated 1162 javac compiler warnings (more than the trunk's current 1155 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.server.blockmanagement.TestDatanodeManager org.apache.hadoop.hdfs.server.namenode.TestFileTruncate Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9815//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/9815//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9815//console This message is automatically generated.
        Hide
        jingzhao Jing Zhao added a comment -

        Thanks for updating the patch, Takanobu.

        So my main concern about the current patch is that to support the subclass we need to bring in extra complexity when using the Builder pattern. Also LocatedBlock is not immutable. Maybe we can avoid this complexity by not using the Builder pattern but simply removing some unnecessary constructors? Looking into the code, seems that we have chance to remove the following constructors:

        1. LocatedBlock(ExtendedBlock, DatanodeInfo[], long, boolean)
          This constructor is called in 4 places, and 3 of them pass -1 and false for the last two arguments. LocatedBlocks#findBlock can also call LocatedBlock(ExtendedBlock, DatanodeInfo[] instead since it sets the offset immediately after creating the object.
        2. LocatedBlock(ExtendedBlock, DatanodeStorageInfo[])
          This constructor is only used once and we can simply use LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean to replace it
        3. LocatedBlock(ExtendedBlock b, DatanodeInfo[], String[], StorageType[])
          This one is also only called by 1 or 2 places and can be replaced by LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean

        In this way we can remove 3 constructors without bringing extra complexity. Thoughts?

        Show
        jingzhao Jing Zhao added a comment - Thanks for updating the patch, Takanobu. So my main concern about the current patch is that to support the subclass we need to bring in extra complexity when using the Builder pattern. Also LocatedBlock is not immutable. Maybe we can avoid this complexity by not using the Builder pattern but simply removing some unnecessary constructors? Looking into the code, seems that we have chance to remove the following constructors: LocatedBlock(ExtendedBlock, DatanodeInfo[], long, boolean) This constructor is called in 4 places, and 3 of them pass -1 and false for the last two arguments. LocatedBlocks#findBlock can also call LocatedBlock(ExtendedBlock, DatanodeInfo[] instead since it sets the offset immediately after creating the object. LocatedBlock(ExtendedBlock, DatanodeStorageInfo[]) This constructor is only used once and we can simply use LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean to replace it LocatedBlock(ExtendedBlock b, DatanodeInfo[], String[], StorageType[]) This one is also only called by 1 or 2 places and can be replaced by LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean In this way we can remove 3 constructors without bringing extra complexity. Thoughts?
        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        Thank you for the helpful advice! I agree with your plan. I think it is more simple and more consistent than to implement builder. I will change my patch with the plan.

        Show
        tasanuma0829 Takanobu Asanuma added a comment - Thank you for the helpful advice! I agree with your plan. I think it is more simple and more consistent than to implement builder. I will change my patch with the plan.
        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        I would like to report my thoughts.

        1. LocatedBlock(ExtendedBlock, DatanodeInfo[], long, boolean)
        I found other places with pass is not -1 and false for the last two arguments.
        TestDFSClientRetries.FailNTimesAnswer.makeBadBlockList, TestDFSUtil.testLocatedBlocks2Locations, TestPBHelper.createLocatedBlockNoStorageMedia
        So it is difficult to remove this constructor.

        2. LocatedBlock(ExtendedBlock, DatanodeStorageInfo[])
        This can be replaced by other constructor.

        3. LocatedBlock(ExtendedBlock b, DatanodeInfo[], String[], StorageType[])
        This is difficult to replaced by LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean).
        Because it is need more information to create instances of DatanodeStorageInfo.
        Conversely, LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean) can be replaced by LocatedBlock(ExtendedBlock b, DatanodeInfo[], String[], StorageType[]).

        So I think we can remove LocatedBlock(ExtendedBlock, DatanodeStorageInfo[]) and LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean). Please would you check it?

        Show
        tasanuma0829 Takanobu Asanuma added a comment - I would like to report my thoughts. 1. LocatedBlock(ExtendedBlock, DatanodeInfo[], long, boolean) I found other places with pass is not -1 and false for the last two arguments. TestDFSClientRetries.FailNTimesAnswer.makeBadBlockList , TestDFSUtil.testLocatedBlocks2Locations , TestPBHelper.createLocatedBlockNoStorageMedia So it is difficult to remove this constructor. 2. LocatedBlock(ExtendedBlock, DatanodeStorageInfo[]) This can be replaced by other constructor. 3. LocatedBlock(ExtendedBlock b, DatanodeInfo[], String[], StorageType[]) This is difficult to replaced by LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean) . Because it is need more information to create instances of DatanodeStorageInfo . Conversely, LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean) can be replaced by LocatedBlock(ExtendedBlock b, DatanodeInfo[], String[], StorageType[]) . So I think we can remove LocatedBlock(ExtendedBlock, DatanodeStorageInfo[]) and LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean) . Please would you check it?
        Hide
        jingzhao Jing Zhao added a comment -

        Sorry for the late response, Takanobu Asanuma.

        For LocatedBlock(ExtendedBlock, DatanodeInfo[], long, boolean), since all the places you mentioned are tests, we can still replace them with LocatedBlock(ExtendedBlock, DatanodeInfo[]. You can use setter methods to set offset and corrupt if necessary.

        For LocatedBlock(ExtendedBlock b, DatanodeInfo[], String[], StorageType[]), I guess all the other information used by DatanodeStorageInfo's construction will not be used by LocatedBlock. But I'm also fine if you decide to keep this constructor. I think we may want to keep LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean) since DatanodeStorageInfo should be the type used by the API after all the heterogeneous storage work.

        Show
        jingzhao Jing Zhao added a comment - Sorry for the late response, Takanobu Asanuma . For LocatedBlock(ExtendedBlock, DatanodeInfo[], long, boolean) , since all the places you mentioned are tests, we can still replace them with LocatedBlock(ExtendedBlock, DatanodeInfo[] . You can use setter methods to set offset and corrupt if necessary. For LocatedBlock(ExtendedBlock b, DatanodeInfo[], String[], StorageType[]) , I guess all the other information used by DatanodeStorageInfo 's construction will not be used by LocatedBlock. But I'm also fine if you decide to keep this constructor. I think we may want to keep LocatedBlock(ExtendedBlock, DatanodeStorageInfo[], long, boolean) since DatanodeStorageInfo should be the type used by the API after all the heterogeneous storage work.
        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        Thank you for your review, Jing. I thought and changed my patch:

        1. I removed some constructors:LocatedBlock(ExtendedBlock, DatanodeInfo[], long, boolean),LocatedBlock(ExtendedBlock, DatanodeStorageInfo[]) and changed callers.
        And I kept LocatedBlock(ExtendedBlock b, DatanodeInfo[], String[], StorageType[]).

        2. Since setStartOffset and setCorrupt are used by tests out of the package, I set public to them.

        3. I set final to storageIDs and storageTypes.

        How does that look?

        Show
        tasanuma0829 Takanobu Asanuma added a comment - Thank you for your review, Jing. I thought and changed my patch: 1. I removed some constructors: LocatedBlock(ExtendedBlock, DatanodeInfo[], long, boolean) , LocatedBlock(ExtendedBlock, DatanodeStorageInfo[]) and changed callers. And I kept LocatedBlock(ExtendedBlock b, DatanodeInfo[], String[], StorageType[]) . 2. Since setStartOffset and setCorrupt are used by tests out of the package, I set public to them. 3. I set final to storageIDs and storageTypes . How does that look?
        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        Jnkins did not run. I submit my patch again to start another Jenkins build.

        Show
        tasanuma0829 Takanobu Asanuma added a comment - Jnkins did not run. I submit my patch again to start another Jenkins build.
        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/12705237/HDFS-7829.4.patch
        against trunk revision 968425e.

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

        +1 tests included. The patch appears to include 3 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.server.namenode.TestListCorruptFileBlocks

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9950//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9950//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/12705237/HDFS-7829.4.patch against trunk revision 968425e. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 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.server.namenode.TestListCorruptFileBlocks Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9950//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9950//console This message is automatically generated.
        Hide
        jingzhao Jing Zhao added a comment -

        The latest patch looks good to me. +1.

        I've committed this to trunk and branch-2. Thanks for the contribution, Takanobu Asanuma!

        Show
        jingzhao Jing Zhao added a comment - The latest patch looks good to me. +1. I've committed this to trunk and branch-2. Thanks for the contribution, Takanobu Asanuma !
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #7384 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7384/)
        HDFS-7829. Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #7384 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7384/ ) HDFS-7829 . Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #139 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/139/)
        HDFS-7829. Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #139 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/139/ ) HDFS-7829 . Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk #873 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/873/)
        HDFS-7829. Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
        • 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 #873 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/873/ ) HDFS-7829 . Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk #2089 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2089/)
        HDFS-7829. Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java
        • 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/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #2089 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2089/ ) HDFS-7829 . Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java 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/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2071 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2071/)
        HDFS-7829. Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java
        • 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/TestDFSUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2071 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2071/ ) HDFS-7829 . Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java 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/TestDFSUtil.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #130 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/130/)
        HDFS-7829. Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #130 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/130/ ) HDFS-7829 . Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #139 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/139/)
        HDFS-7829. Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #139 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/139/ ) HDFS-7829 . Code clean up for LocatedBlock. Contributed by Takanobu Asanuma. (jing9: rev a6a5aae472d015d2ea5cd746719485dff93873a8) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlocks.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSClientRetries.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockRecoveryCommand.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LocatedBlock.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java
        Hide
        tasanuma0829 Takanobu Asanuma added a comment -

        Thank you for your all help, Jing!

        Show
        tasanuma0829 Takanobu Asanuma added a comment - Thank you for your all help, Jing!

          People

          • Assignee:
            tasanuma0829 Takanobu Asanuma
            Reporter:
            jingzhao Jing Zhao
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development