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

      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.002.patch
        33 kB
        Jing Zhao
      2. HADOOP-8820.b1.003.patch
        33 kB
        Jing Zhao
      3. HADOOP-8820.patch
        33 kB
        Junping Du

        Issue Links

          Activity

          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.
          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.
          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
          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?
          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?
          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
          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
          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.
          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 -

          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
          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 -

          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.
          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
          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 -

          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
          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 -

          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
          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 -

          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
          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 -

          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 -

          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
          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
          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
          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 -

          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!
          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
          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:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development