Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-3942

Backport HDFS-3495: Update balancer policy for Network Topology with additional 'NodeGroup' layer

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0
    • Fix Version/s: 1.2.0, 1-win
    • Component/s: balancer
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Target Version/s:

      Description

      This is the backport work for HDFS-3495 and HDFS-4234.

      1. HDFS-3942.patch
        29 kB
        Junping Du
      2. HDFS-3942-v2.patch
        37 kB
        Junping Du
      3. HDFS-3942-v3.patch
        38 kB
        Junping Du
      4. HDFS-3942-v4.patch
        38 kB
        Junping Du
      5. HDFS-3942-v5.patch
        44 kB
        Junping Du

        Issue Links

          Activity

          Hide
          Suresh Srinivas added a comment -

          Target version cannot be 1.0.3. It is already released.

          Show
          Suresh Srinivas added a comment - Target version cannot be 1.0.3. It is already released.
          Hide
          Junping Du added a comment -

          Update patch due to refactor work for balancer in trunk branch.

          Show
          Junping Du added a comment - Update patch due to refactor work for balancer in trunk branch.
          Hide
          Junping Du added a comment -

          Thanks Suresh for reminder. I just update target version to 1.1.2.

          Show
          Junping Du added a comment - Thanks Suresh for reminder. I just update target version to 1.1.2.
          Hide
          Junping Du added a comment -

          This patch backport HDFS-4234 too.

          Show
          Junping Du added a comment - This patch backport HDFS-4234 too.
          Hide
          Tsz Wo Nicholas Sze added a comment -
          • Remove the commented code in chooseNodes().
          • Move getNetworkTopologyInstance to NetworkTopology.
          • Remove areDataNodesOnSameNodeGroup(..).

          BTW, should we also wait for HDFS-4261?

          Show
          Tsz Wo Nicholas Sze added a comment - Remove the commented code in chooseNodes(). Move getNetworkTopologyInstance to NetworkTopology. Remove areDataNodesOnSameNodeGroup(..). BTW, should we also wait for HDFS-4261 ?
          Hide
          Junping Du added a comment -

          Thanks Nicholas for review and comments. I will address these comments later. In branch-1, there is no multiple NameNodeConnector, so the issue for HDFS-4261 is not existing here. Thoughts?

          Show
          Junping Du added a comment - Thanks Nicholas for review and comments. I will address these comments later. In branch-1, there is no multiple NameNodeConnector, so the issue for HDFS-4261 is not existing here. Thoughts?
          Hide
          Junping Du added a comment -

          Address Nicholas' comments in v3 patch.

          Show
          Junping Du added a comment - Address Nicholas' comments in v3 patch.
          Hide
          Junping Du added a comment -

          Remove unnecessary import and code in v4 patch.

          Show
          Junping Du added a comment - Remove unnecessary import and code in v4 patch.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > ... so the issue for HDFS-4261 is not existing here. Thoughts?

          I think you are right. How about also add the new test in HDFS-4261 to branch-1?

          Some more comments on the patch:

          • TestBalancerWithNodeGroup should use junit 4 and not extend TestCase.
          • Change the comment on top of isOnSameNodeGroupWithReplicas(..) to javadoc and copy the javadoc from trunk.
          • In TestBalancerWithNodeGroup, the waitForHeartBeat(..) and runBalancer(..) are at the end but they are put near the beginning in trunk. Could you make order them the same way? In general, please make the code the same for both trunk and branch-1 so that it is easier to maintain.
          Show
          Tsz Wo Nicholas Sze added a comment - > ... so the issue for HDFS-4261 is not existing here. Thoughts? I think you are right. How about also add the new test in HDFS-4261 to branch-1? Some more comments on the patch: TestBalancerWithNodeGroup should use junit 4 and not extend TestCase. Change the comment on top of isOnSameNodeGroupWithReplicas(..) to javadoc and copy the javadoc from trunk. In TestBalancerWithNodeGroup, the waitForHeartBeat(..) and runBalancer(..) are at the end but they are put near the beginning in trunk. Could you make order them the same way? In general, please make the code the same for both trunk and branch-1 so that it is easier to maintain.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          BTW, we have to generate the patch with "--no-prefix" for branch-1. Otherwise, "ant test-patch" won't work. Please also run ant test-patch and all the unit tests. Thanks a lot!

          Show
          Tsz Wo Nicholas Sze added a comment - BTW, we have to generate the patch with "--no-prefix" for branch-1. Otherwise, "ant test-patch" won't work. Please also run ant test-patch and all the unit tests. Thanks a lot!
          Hide
          Junping Du added a comment -

          Thanks Nicholas for the comments. Yes. I will try the new patch on my local branch-1 env (previous issue should be resolved by HADOOP-9051).

          Show
          Junping Du added a comment - Thanks Nicholas for the comments. Yes. I will try the new patch on my local branch-1 env (previous issue should be resolved by HADOOP-9051 ).
          Hide
          Junping Du added a comment -

          Update patch as last comments from Nicholas. Will update test-patch result later.

          Show
          Junping Du added a comment - Update patch as last comments from Nicholas. Will update test-patch result later.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Thanks a lot. I just have run test-patch.

               [exec] -1 overall.  
               [exec] 
               [exec]     +1 @author.  The patch does not contain any @author tags.
               [exec] 
               [exec]     +1 tests included.  The patch appears to include 7 new or modified tests.
               [exec] 
               [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
               [exec] 
               [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
               [exec] 
               [exec]     -1 findbugs.  The patch appears to introduce 11 new Findbugs (version 1.3.9) warnings.
          
          Show
          Tsz Wo Nicholas Sze added a comment - Thanks a lot. I just have run test-patch. [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 7 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] -1 findbugs. The patch appears to introduce 11 new Findbugs (version 1.3.9) warnings.
          Hide
          Junping Du added a comment -

          Thanks Nicholas to post test-patch result. I am curious on are all findbugs issues involved by this patch?
          I have this question because I tried a previous patch on my env (not sure if it is settled down well) to have 225 Findbugs warning. I saw the same Findbugs warning number in recent result from other guys (like Zhao Jing) to post for branch-1 work. Just a little confused.

              -1 findbugs.  The patch appears to introduce 225 new Findbugs (version 2.0.1) warnings.
          
          Show
          Junping Du added a comment - Thanks Nicholas to post test-patch result. I am curious on are all findbugs issues involved by this patch? I have this question because I tried a previous patch on my env (not sure if it is settled down well) to have 225 Findbugs warning. I saw the same Findbugs warning number in recent result from other guys (like Zhao Jing) to post for branch-1 work. Just a little confused. -1 findbugs. The patch appears to introduce 225 new Findbugs (version 2.0.1) warnings.
          Hide
          Jing Zhao added a comment -

          Seems the test-patch in branch-1 has some problem with findbug?

          By the way, I run ant test for the latest patch and it passed all the local unit tests.

          Show
          Jing Zhao added a comment - Seems the test-patch in branch-1 has some problem with findbug? By the way, I run ant test for the latest patch and it passed all the local unit tests.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          It seems that test-patch cannot determine the new findbugs warnings correctly. I usually compare the findbugs warnings with an empty patch which also generate 11 "new" Findbugs (version 1.3.9) warnings.

          Show
          Tsz Wo Nicholas Sze added a comment - It seems that test-patch cannot determine the new findbugs warnings correctly. I usually compare the findbugs warnings with an empty patch which also generate 11 "new" Findbugs (version 1.3.9) warnings.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 on the new patch.

          I have committed this. Thanks Junping!

          Also thanks Jing for testing the patch.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 on the new patch. I have committed this. Thanks Junping! Also thanks Jing for testing the patch.
          Hide
          Junping Du added a comment -

          Thanks Nicholas and Jing for help in review and testing patch!

          Show
          Junping Du added a comment - Thanks Nicholas and Jing for help in review and testing patch!
          Hide
          Junping Du added a comment -

          BTW, I agree that test-patch in branch-1 has problem with findbugs as findbugs seems to accumulate warnings of previous code.
          As lacking of a central pre-commit test (like Trunk), each individual developers' env with different version of FindBugs may have different result which make some warnings get ignored. So I guess it could be useful to have a central Jenkins for test-patch on branch-1 (and branch-2 too)? I saw some effort initiated in HADOOP-7435 but no clear the history and how to follow up.

          Show
          Junping Du added a comment - BTW, I agree that test-patch in branch-1 has problem with findbugs as findbugs seems to accumulate warnings of previous code. As lacking of a central pre-commit test (like Trunk), each individual developers' env with different version of FindBugs may have different result which make some warnings get ignored. So I guess it could be useful to have a central Jenkins for test-patch on branch-1 (and branch-2 too)? I saw some effort initiated in HADOOP-7435 but no clear the history and how to follow up.
          Hide
          Matt Foley added a comment -

          Closed upon release of Hadoop 1.2.0.

          Show
          Matt Foley added a comment - Closed upon release of Hadoop 1.2.0.

            People

            • Assignee:
              Junping Du
              Reporter:
              Junping Du
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development