Hadoop Common
  1. Hadoop Common
  2. HADOOP-8820

Backport HADOOP-8469 and HADOOP-8470: add "NodeGroup" layer in new NetworkTopology (also known as NetworkTopologyWithNodeGroup)

    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: net
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      This patch backport HADOOP-8469 and HADOOP-8470 to branch-1 and includes:
      1. Make NetworkTopology class pluggable for extension.
      2. Implement a 4-layer NetworkTopology class (named as NetworkTopologyWithNodeGroup) to use in virtualized environment (or other situation with additional layer between host and rack).

      1. HADOOP-8820.b1.003.patch
        33 kB
        Jing Zhao
      2. HADOOP-8820.b1.002.patch
        33 kB
        Jing Zhao
      3. HADOOP-8820.patch
        33 kB
        Junping Du

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          33m 45s 1 Junping Du 16/Sep/12 20:17
          Patch Available Patch Available Open Open
          3h 58m 1 Suresh Srinivas 17/Sep/12 00:15
          Open Open Resolved Resolved
          57d 22h 23m 1 Tsz Wo Nicholas Sze 13/Nov/12 21:39
          Resolved Resolved Closed Closed
          182d 7h 36m 1 Matt Foley 15/May/13 06:16
          Matt Foley made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          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.
          Hide
          Junping Du added a comment -

          BTW, it is in HDFS-3495.

          Show
          Junping Du added a comment - BTW, it is in HDFS-3495 .
          Hide
          Junping Du added a comment -

          Nicholas, I have updated balancer patch on trunk. Will you help to take a look at it? Thanks!

          Show
          Junping Du added a comment - Nicholas, I have updated balancer patch on trunk. Will you help to take a look at it? Thanks!
          Tsz Wo Nicholas Sze made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Fix Version/s 1.2.0 [ 12321659 ]
          Fix Version/s 1-win [ 12320361 ]
          Resolution Fixed [ 1 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 on HADOOP-8820.b1.002.patch

          I have committed it. Thanks, Junping and Jing!

          Show
          Tsz Wo Nicholas Sze added a comment - +1 on HADOOP-8820 .b1.002.patch I have committed it. Thanks, Junping and Jing!
          Hide
          Junping Du added a comment -

          Great. Thanks Nicholas! Also, Thanks Jing for review and verification.

          Show
          Junping Du added a comment - Great. Thanks Nicholas! Also, Thanks Jing for review and verification.
          Hide
          Jing Zhao added a comment -

          By the way, I've run unit tests locally for both 002 and 003 patches, and all the tests passed except TestFileCreation, which should be unrelated and has been reported in HDFS-4180.

          Show
          Jing Zhao added a comment - By the way, I've run unit tests locally for both 002 and 003 patches, and all the tests passed except TestFileCreation, which should be unrelated and has been reported in HDFS-4180 .
          Hide
          Tsz Wo Nicholas Sze added a comment -

          That's great. Thanks, Junping. Then, I will first commit HADOOP-8820.b1.002.patch. For the additional bug fixes, let's create a new JIRA since we also need to fix the code in trunk.

          Show
          Tsz Wo Nicholas Sze added a comment - That's great. Thanks, Junping. Then, I will first commit HADOOP-8820 .b1.002.patch. For the additional bug fixes, let's create a new JIRA since we also need to fix the code in trunk.
          Hide
          Junping Du added a comment -

          Ok. Sounds good. I will split the patch into 4 patches. Thanks. Nicholas!

          Show
          Junping Du added a comment - Ok. Sounds good. I will split the patch into 4 patches. Thanks. Nicholas!
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Junping, there is no easy way to review such a big patch in HADOOP-8817. It is simply too big. Also, branch-1 is a stable branch so that we must be conservative. Splitting it into 4 parts HADOOP-8820, HDFS-3941, HDFS-3942 and MAPREDUCE-4660 sounds really good. Would you mind doing it?

          In the meantime, I will review your balancer patch for trunk.

          Show
          Tsz Wo Nicholas Sze added a comment - Junping, there is no easy way to review such a big patch in HADOOP-8817 . It is simply too big. Also, branch-1 is a stable branch so that we must be conservative. Splitting it into 4 parts HADOOP-8820 , HDFS-3941 , HDFS-3942 and MAPREDUCE-4660 sounds really good. Would you mind doing it? In the meantime, I will review your balancer patch for trunk.
          Hide
          Junping Du added a comment -

          Hi,Jing. You are right that firstGrandChild is not checked before, I was thinking you are talking about firstChild. Also, the 4 methods you are listing here is just replicated. I already address your comments in v4 patch in HADOOP-8817. Thanks!

          Show
          Junping Du added a comment - Hi,Jing. You are right that firstGrandChild is not checked before, I was thinking you are talking about firstChild. Also, the 4 methods you are listing here is just replicated. I already address your comments in v4 patch in HADOOP-8817 . Thanks!
          Hide
          Jing Zhao added a comment -

          The 1st is not a issue as children is already checked before.

          I'm not sure if I understand the code correctly here. So the isRack() function first checks the children of the current node ("if (getChildren().isEmpty())"), but does not check the children of the first child of the current node. Thus is it possible that "((InnerNode) firstChild).children" is empty (in which case the following get(0) may cause an IndexOutofBoundsException)? Or somewhere outside the function it has been checked?

          By the way, in the latest patch in HADOOP-8817 (HADOOP-8817-v3.patch), several method definitions seem to be repeated between BlockPlacementPolicy.java and BlockPlacementPolicyDefault.java (e.g., adjustSetsWithChosenReplica(), splitNodesWithLocalityGroup(), getLocalityGroupForSplit() and getRack()). So do we need to recheck the patch to verify?

          Show
          Jing Zhao added a comment - The 1st is not a issue as children is already checked before. I'm not sure if I understand the code correctly here. So the isRack() function first checks the children of the current node ("if (getChildren().isEmpty())"), but does not check the children of the first child of the current node. Thus is it possible that "((InnerNode) firstChild).children" is empty (in which case the following get(0) may cause an IndexOutofBoundsException)? Or somewhere outside the function it has been checked? By the way, in the latest patch in HADOOP-8817 ( HADOOP-8817 -v3.patch), several method definitions seem to be repeated between BlockPlacementPolicy.java and BlockPlacementPolicyDefault.java (e.g., adjustSetsWithChosenReplica(), splitNodesWithLocalityGroup(), getLocalityGroupForSplit() and getRack()). So do we need to recheck the patch to verify?
          Hide
          Junping Du added a comment -

          Nicholas, Thanks for comments and questions. Under your great help, most HDFS part of code is committed into trunk (except Balancer), so the left parts missing in trunk are mostly YARN. HADOOP-8817 patch is based on MRV1 which has less code change comparing with YARN and MapReduce part is already reviewed (by Luke). Also, I did a lot of tests on a deployed virtualized hadoop cluster for combined patch, so I think it could be better to commit these patches as a whole patch to get rid of potential bug involved by sync and could be convenient for the patch to go to release branch (branch-1.2?). What do you think?

          Show
          Junping Du added a comment - Nicholas, Thanks for comments and questions. Under your great help, most HDFS part of code is committed into trunk (except Balancer), so the left parts missing in trunk are mostly YARN. HADOOP-8817 patch is based on MRV1 which has less code change comparing with YARN and MapReduce part is already reviewed (by Luke). Also, I did a lot of tests on a deployed virtualized hadoop cluster for combined patch, so I think it could be better to commit these patches as a whole patch to get rid of potential bug involved by sync and could be convenient for the patch to go to release branch (branch-1.2?). What do you think?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Junping, thanks for the update. I also want to commit the HADOOP-8817 patch directly. However, some parts of the patch is not yet committed to trunk. Your original plan is to have separated patches for various components. I think it is a good plan since it is easier to review the patches. Why don't we stick with it?

          Jing, thanks for helping out here.

          Show
          Tsz Wo Nicholas Sze added a comment - Junping, thanks for the update. I also want to commit the HADOOP-8817 patch directly. However, some parts of the patch is not yet committed to trunk. Your original plan is to have separated patches for various components. I think it is a good plan since it is easier to review the patches. Why don't we stick with it? Jing, thanks for helping out here.
          Hide
          Junping Du added a comment -

          Also, forget to note that, I think it could be convenient to have only one patch for "NodeGroup" layer with both HDFS and MAPREDUCE. So I suggest we don't commit this patch but commit HADOOP-8817 directly.

          Show
          Junping Du added a comment - Also, forget to note that, I think it could be convenient to have only one patch for "NodeGroup" layer with both HDFS and MAPREDUCE. So I suggest we don't commit this patch but commit HADOOP-8817 directly.
          Hide
          Junping Du added a comment -

          On above two issues you point out, I think 2nd is potentially a issue and you can file a JIRA to follow up. The 1st is not a issue as children is already checked before.

          Show
          Junping Du added a comment - On above two issues you point out, I think 2nd is potentially a issue and you can file a JIRA to follow up. The 1st is not a issue as children is already checked before.
          Hide
          Junping Du added a comment -

          Hi Jing, can you post "ant test" result on HADOOP-8817? I just adjust the patch due to Nicholas' comments.

          Show
          Junping Du added a comment - Hi Jing, can you post "ant test" result on HADOOP-8817 ? I just adjust the patch due to Nicholas' comments.
          Hide
          Jing Zhao added a comment -

          Hi Junping, could you please recheck the new patch and make sure all the corresponding changes in HADOOP-8817 have been included? Thanks.

          Show
          Jing Zhao added a comment - Hi Junping, could you please recheck the new patch and make sure all the corresponding changes in HADOOP-8817 have been included? Thanks.
          Jing Zhao made changes -
          Attachment HADOOP-8820.b1.003.patch [ 12553249 ]
          Hide
          Jing Zhao added a comment -

          New patch based on HADOOP-8817 and Nicholas's comments. Will run ant test for the new patch.

          Show
          Jing Zhao added a comment - New patch based on HADOOP-8817 and Nicholas's comments. Will run ant test for the new patch.
          Hide
          Jing Zhao added a comment -

          Sure. I will re-generate the patch based on HADOOP-8817 and run ant test for the new patch. Can you give more details about the changes you made in 8817? Thanks.

          Show
          Jing Zhao added a comment - Sure. I will re-generate the patch based on HADOOP-8817 and run ant test for the new patch. Can you give more details about the changes you made in 8817? Thanks.
          Hide
          Junping Du added a comment -

          Thanks Jing for the help here. However, I think the patch in HADOOP-8817 is the latest code and it fix a bug on placing replicas (the first repilca's host is not disallowed before). I did a lot of test (functional and performance) in a real deployed cluster on that patch, but my "ant test" still meet some trouble on my environment. Can you help to run ant test on that patch?

          Show
          Junping Du added a comment - Thanks Jing for the help here. However, I think the patch in HADOOP-8817 is the latest code and it fix a bug on placing replicas (the first repilca's host is not disallowed before). I did a lot of test (functional and performance) in a real deployed cluster on that patch, but my "ant test" still meet some trouble on my environment. Can you help to run ant test on that patch?
          Hide
          Jing Zhao added a comment -

          Besides, some other small issues:
          1.

          +    @Override
          +    boolean isRack() {
          +      // it is node group
          +      if (getChildren().isEmpty()) {
          +        return false;
          +      }
          +
          +      Node firstChild = children.get(0);
          +
          +      if (firstChild instanceof InnerNode) {
          +        Node firstGrandChild = (((InnerNode) firstChild).children).get(0);
          +        if (firstGrandChild instanceof InnerNode) {
          

          I think here maybe firstChild.children can be empty thus the get(0) may cause an IndexOutofBoundsException. So maybe we need to check if children is empty first (and if it is, we can return true)?

          2.

          +  public String getNodeGroup(String loc) {
          +    try {
          +      netlock.readLock().lock();
          +      loc = InnerNode.normalize(loc);
          +      Node locNode = getNode(loc);
          +      if (locNode instanceof InnerNodeWithNodeGroup) {
          +        InnerNodeWithNodeGroup node = (InnerNodeWithNodeGroup) locNode;
          +        if (node.isNodeGroup()) {
          +          return loc;
          +        } else if (node.isRack()) {
          +          // not sure the node group for a rack
          +          return null;
          +        } else {
          +          // may be a leaf node
          +          return getNodeGroup(node.getNetworkLocation());
          +        }
          

          Since the "locNode" is already an instance of InnerNodeWithNodeGroup, I think it cannot be a leaf node anymore, thus the code may never run into the "else" part?

          I can create a separate jira to fix if you think the two issues stand.

          Show
          Jing Zhao added a comment - Besides, some other small issues: 1. + @Override + boolean isRack() { + // it is node group + if (getChildren().isEmpty()) { + return false; + } + + Node firstChild = children.get(0); + + if (firstChild instanceof InnerNode) { + Node firstGrandChild = (((InnerNode) firstChild).children).get(0); + if (firstGrandChild instanceof InnerNode) { I think here maybe firstChild.children can be empty thus the get(0) may cause an IndexOutofBoundsException. So maybe we need to check if children is empty first (and if it is, we can return true)? 2. + public String getNodeGroup(String loc) { + try { + netlock.readLock().lock(); + loc = InnerNode.normalize(loc); + Node locNode = getNode(loc); + if (locNode instanceof InnerNodeWithNodeGroup) { + InnerNodeWithNodeGroup node = (InnerNodeWithNodeGroup) locNode; + if (node.isNodeGroup()) { + return loc; + } else if (node.isRack()) { + // not sure the node group for a rack + return null; + } else { + // may be a leaf node + return getNodeGroup(node.getNetworkLocation()); + } Since the "locNode" is already an instance of InnerNodeWithNodeGroup, I think it cannot be a leaf node anymore, thus the code may never run into the "else" part? I can create a separate jira to fix if you think the two issues stand.
          Jing Zhao made changes -
          Attachment HADOOP-8820.b1.002.patch [ 12553234 ]
          Hide
          Jing Zhao added a comment -

          Hi Junping, maybe you're busy with other stuff. Since to address Nicholas's comments only needs simple changes, I just make these changes on your patch and run unit tests for you. All the test cases have passed on my machine. Thanks!

          Show
          Jing Zhao added a comment - Hi Junping, maybe you're busy with other stuff. Since to address Nicholas's comments only needs simple changes, I just make these changes on your patch and run unit tests for you. All the test cases have passed on my machine. Thanks!
          Hide
          Junping Du added a comment -

          Thanks Nicholas for reviewing. I will address your comments in HADOOP-8817 where I put all 4 patches together. Hopefully, it is easy to review and maintain.

          Show
          Junping Du added a comment - Thanks Nicholas for reviewing. I will address your comments in HADOOP-8817 where I put all 4 patches together. Hopefully, it is easy to review and maintain.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > ... how does this patch marked as patch available for review?
          You may simply post the patch on the JIRA.

          The patch looks good. Some minor comments:

          • The conf property net.topology.nodegroup.aware is not used. It is also not in the HADOOP-8469 and HADOOP-8470 patches. Let's add it in later patches.
          • The netlock.writeLock().lock() below should be before try{}.
            //NetworkTopologyWithNodeGroup.add(..)
            +    try {
            +      netlock.writeLock().lock();
            +      Node rack = null;
            

          Could you also run test-patch and all the unit tests with the new patch?

          Show
          Tsz Wo Nicholas Sze added a comment - > ... how does this patch marked as patch available for review? You may simply post the patch on the JIRA. The patch looks good. Some minor comments: The conf property net.topology.nodegroup.aware is not used. It is also not in the HADOOP-8469 and HADOOP-8470 patches. Let's add it in later patches. The netlock.writeLock().lock() below should be before try{}. //NetworkTopologyWithNodeGroup.add(..) + try { + netlock.writeLock().lock(); + Node rack = null ; Could you also run test-patch and all the unit tests with the new patch?
          Junping Du made changes -
          Description This patch backport HADOOP-8469 and HADOOP-8470 to branch-1 and includes:
          1. Make NetworkTopology class pluggable for extension.
          2. Implement a 4-layer NetworkTopology class (named as NetworkTopologyWithNodeGroup) to use in virtualized environment (or other situation with additional layer between host and rack).
          Junping Du made changes -
          Link This issue blocks MAPREDUCE-4660 [ MAPREDUCE-4660 ]
          Junping Du made changes -
          Link This issue blocks HDFS-3942 [ HDFS-3942 ]
          Junping Du made changes -
          Link This issue blocks HDFS-3941 [ HDFS-3941 ]
          Junping Du made changes -
          Link This issue Is contained by HADOOP-8817 [ HADOOP-8817 ]
          Junping Du made changes -
          Link This issue is related to HADOOP-8817 [ HADOOP-8817 ]
          Junping Du made changes -
          Target Version/s 1.0.0 [ 12318244 ] 1.2.0 [ 12321659 ]
          Hide
          Junping Du added a comment -

          Thanks Suresh. Then, how does this patch marked as patch available for review?

          Show
          Junping Du added a comment - Thanks Suresh. Then, how does this patch marked as patch available for review?
          Suresh Srinivas made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Suresh Srinivas added a comment -

          Jenkins build does not pickup patch for 1.x. Canceling the patch.

          Show
          Suresh Srinivas added a comment - Jenkins build does not pickup patch for 1.x. Canceling the patch.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12545333/HADOOP-8820.patch
          against trunk revision .

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

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1476//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12545333/HADOOP-8820.patch against trunk revision . -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1476//console This message is automatically generated.
          Junping Du made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Junping Du added a comment -

          This patch is for branch-1 only.

          Show
          Junping Du added a comment - This patch is for branch-1 only.
          Junping Du made changes -
          Attachment HADOOP-8820.patch [ 12545333 ]
          Junping Du made changes -
          Link This issue is related to HADOOP-8817 [ HADOOP-8817 ]
          Junping Du made changes -
          Field Original Value New Value
          Assignee Junping Du [ djp ]
          Junping Du created issue -

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development