Hadoop Common
  1. Hadoop Common
  2. HADOOP-1085

Remove 'port rolling' from Mini{DFS|MR}Cluster

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.12.0
    • Fix Version/s: 0.13.0
    • Component/s: test
    • Labels:
      None

      Description

      The rolling of ports in these 2 clusters lead to a lot of timing issues and failed test cases; as witnessed in our patch process.

      The way around is to let the OS pick the port for the NameNode/JobTracker and let the the DataNode/TaskTracker query them for the port to connect to and then use that port.

      1. HADOOP-1085_20070326_2.patch
        54 kB
        Arun C Murthy
      2. HADOOP-1085_20070311_1.patch
        53 kB
        Arun C Murthy

        Issue Links

          Activity

          Arun C Murthy created issue -
          Hide
          Doug Cutting added a comment -

          This can wait until 0.13.

          Show
          Doug Cutting added a comment - This can wait until 0.13.
          Doug Cutting made changes -
          Field Original Value New Value
          Fix Version/s 0.12.1 [ 12312347 ]
          Fix Version/s 0.13.0 [ 12312348 ]
          Priority Critical [ 2 ] Major [ 3 ]
          Hide
          Arun C Murthy added a comment -

          Here is a patch which removes 'port rolling' from the test-cases... a bit of a work in progress since it seems to weirdly cause some streaming tests to fail.

          Highlights:
          1. Removes port-rolling by passing port '0' and letting the OS pick a ephemeral port.
          2. Changed org.apache.hadoop.ipc.Server to bind in its constructor and not in the 'run' method to be sure of the ephemeral port.
          3. Since the 'port' changes from the one passed (which previously is stored in some data-members/configuration), this resets them in all necessary places.
          4. Removed support for 'dataNodeFirst/taskTrackerFirst' flag to the Mini

          {DFS|MR}Cluster since they cannot be supported now (we can't know the NameNode/JobTracker ephemeral ports until we start them).
          5. Fixed all the test-cases and deprecated the unecessary constructors in Mini{DFS|MR}

          Cluster which cannot be supported.

          Show
          Arun C Murthy added a comment - Here is a patch which removes 'port rolling' from the test-cases... a bit of a work in progress since it seems to weirdly cause some streaming tests to fail. Highlights: 1. Removes port-rolling by passing port '0' and letting the OS pick a ephemeral port. 2. Changed org.apache.hadoop.ipc.Server to bind in its constructor and not in the 'run' method to be sure of the ephemeral port. 3. Since the 'port' changes from the one passed (which previously is stored in some data-members/configuration), this resets them in all necessary places. 4. Removed support for 'dataNodeFirst/taskTrackerFirst' flag to the Mini {DFS|MR}Cluster since they cannot be supported now (we can't know the NameNode/JobTracker ephemeral ports until we start them). 5. Fixed all the test-cases and deprecated the unecessary constructors in Mini{DFS|MR} Cluster which cannot be supported.
          Arun C Murthy made changes -
          Attachment HADOOP-1085_20070311_1.patch [ 12353072 ]
          Hide
          Konstantin Shvachko added a comment -

          I am running "ant test-core" with this patch and without.
          Here are my results
          =============================================
          Without 1085:

          BUILD SUCCESSFUL
          Total time: 19 minutes 2 seconds

          =============================================
          With 1085 patch:

          [junit] Running org.apache.hadoop.dfs.TestReplication
          [junit] Tests run: 1, Failures: 0, Errors: 1, Time elapsed: 29.525 sec
          [junit] Test org.apache.hadoop.dfs.TestReplication FAILED

          BUILD FAILED

          /home/shv/kryptonite/hadoop/build.xml:461: Tests failed!
          Total time: 19 minutes 18 seconds
          =============================================

          1) It breaks TestReplication
          2) It runs slower.
          3) Looking at the code I don't see how it eliminates dataNodeFirst parameter
          There is a public construtor (not deprecated)
          public MiniDFSCluster(int namenodePort,
          Configuration conf,
          int nDatanodes,
          boolean dataNodeFirst,
          boolean formatNamenode,
          String[] racks) throws IOException {
          and code inside supports the parameter.
          if (dataNodeFirst)

          { startDataNodes(conf, racks, data_dir); }

          4) I, like Owen, don't like sleeps. But this patch still uses them.
          System.err.println("Waiting for the NameNode to initialize...");
          Thread.sleep(1000);

          Show
          Konstantin Shvachko added a comment - I am running "ant test-core" with this patch and without. Here are my results ============================================= Without 1085: BUILD SUCCESSFUL Total time: 19 minutes 2 seconds ============================================= With 1085 patch: [junit] Running org.apache.hadoop.dfs.TestReplication [junit] Tests run: 1, Failures: 0, Errors: 1, Time elapsed: 29.525 sec [junit] Test org.apache.hadoop.dfs.TestReplication FAILED BUILD FAILED /home/shv/kryptonite/hadoop/build.xml:461: Tests failed! Total time: 19 minutes 18 seconds ============================================= 1) It breaks TestReplication 2) It runs slower. 3) Looking at the code I don't see how it eliminates dataNodeFirst parameter There is a public construtor (not deprecated) public MiniDFSCluster(int namenodePort, Configuration conf, int nDatanodes, boolean dataNodeFirst, boolean formatNamenode, String[] racks) throws IOException { and code inside supports the parameter. if (dataNodeFirst) { startDataNodes(conf, racks, data_dir); } 4) I, like Owen, don't like sleeps. But this patch still uses them. System.err.println("Waiting for the NameNode to initialize..."); Thread.sleep(1000);
          Hide
          Jim Kellerman added a comment -

          +0

          I happen to like the way MiniDFSCluster works. I don't have to worry about modifying configs to get it to work. As I am building applications that run on top of MiniDFSCluster the status quo is a big win for me.

          Show
          Jim Kellerman added a comment - +0 I happen to like the way MiniDFSCluster works. I don't have to worry about modifying configs to get it to work. As I am building applications that run on top of MiniDFSCluster the status quo is a big win for me.
          Arun C Murthy made changes -
          Link This issue is blocked by HADOOP-1047 [ HADOOP-1047 ]
          Hide
          Arun C Murthy added a comment -

          @Konstantin
          1. I tested this patch using HADOOP-1047 which seemed to solve TestReplication's failure, it's out of sync with trunk now - anyway I've linked it to this issue to keep track.
          3. The idea is to keep the dataNodeFirst (and equivalently taskTrackerFirst for MiniMRCluster) so that people can manually run them if needed. However the automated test cases don't use it; and hence that constructor isn't deprecated.
          4. The sleep there is a busy wait till the namenode comes up. I agree with both of you that we shouldn't use them to solve timing issues, which isn't the case here. We absolutely need to wait for the NameNode to start so that we can then query it and start the datanodes.

          @Jim
          Jim, you wouldn't have to modify any config to get it to work, it is, as before, completely automated (we just let the OS pick the ports). Maybe I misunderstand your concerns?

          Show
          Arun C Murthy added a comment - @Konstantin 1. I tested this patch using HADOOP-1047 which seemed to solve TestReplication's failure, it's out of sync with trunk now - anyway I've linked it to this issue to keep track. 3. The idea is to keep the dataNodeFirst (and equivalently taskTrackerFirst for MiniMRCluster) so that people can manually run them if needed. However the automated test cases don't use it; and hence that constructor isn't deprecated. 4. The sleep there is a busy wait till the namenode comes up. I agree with both of you that we shouldn't use them to solve timing issues, which isn't the case here. We absolutely need to wait for the NameNode to start so that we can then query it and start the datanodes. @Jim Jim, you wouldn't have to modify any config to get it to work, it is, as before, completely automated (we just let the OS pick the ports). Maybe I misunderstand your concerns?
          Hide
          Arun C Murthy added a comment -

          Nigel, can you try anc check if this works better for the patch process? Thanks!

          Show
          Arun C Murthy added a comment - Nigel, can you try anc check if this works better for the patch process? Thanks!
          Arun C Murthy made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Nigel Daley added a comment -

          Arun, this patch no longer applies to trunk. Can you resubmit it? I think it makes sense to get a new patch for this issue committed as soon as possible given that it should help with the stability of our unit tests. Once this is committed, I will re-enable our patch process.

          Show
          Nigel Daley added a comment - Arun, this patch no longer applies to trunk. Can you resubmit it? I think it makes sense to get a new patch for this issue committed as soon as possible given that it should help with the stability of our unit tests. Once this is committed, I will re-enable our patch process.
          Nigel Daley made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Konstantin Shvachko added a comment -

          = You say that dataNodeFirst cannot be supported, but your code first allows the parameter and second retains old processing of it.
          Data-nodes will not find the name-node if somebody calls the public constructor with dataNodeFirst=true.
          This is a potential bug. I think you should remove the parameter.

          = Looking at the name-node code. You are passing the actual name-node ports by modifying the configuration parameter
          of the name-node constructor, which makes it an output parameter. I see three problems with that approach:
          a) it is not documented;
          b) if somebody uses a defensive copy (as Nigel calls it) of the config parameter, meaning somebody who does not trust
          the name-node to modify its config, there is no way to get the actual ports directly from the name-node instance.
          c) in a real cluster if I configure fs.default.name = 0 and/or dfs.info.port = 0 then data-nodes will never find the name-node.

          I'd propose to create a separate constructor for the NameNode that mini cluster would use.
          I propose not to modify the config inside NameNode but rather use
          NameNode.getNameNodeAddress() and FSNamesystem.getNameNodeInfoPort()
          inside the mini cluster

          Show
          Konstantin Shvachko added a comment - = You say that dataNodeFirst cannot be supported, but your code first allows the parameter and second retains old processing of it. Data-nodes will not find the name-node if somebody calls the public constructor with dataNodeFirst=true. This is a potential bug. I think you should remove the parameter. = Looking at the name-node code. You are passing the actual name-node ports by modifying the configuration parameter of the name-node constructor, which makes it an output parameter. I see three problems with that approach: a) it is not documented; b) if somebody uses a defensive copy (as Nigel calls it) of the config parameter, meaning somebody who does not trust the name-node to modify its config, there is no way to get the actual ports directly from the name-node instance. c) in a real cluster if I configure fs.default.name = 0 and/or dfs.info.port = 0 then data-nodes will never find the name-node. I'd propose to create a separate constructor for the NameNode that mini cluster would use. I propose not to modify the config inside NameNode but rather use NameNode.getNameNodeAddress() and FSNamesystem.getNameNodeInfoPort() inside the mini cluster
          Hide
          Arun C Murthy added a comment -

          Here is an updated patch to reflect changes to trunk...

          Konstantin: Appreciate your review. I've had to leave in the code to modify the 'conf' since it is passed to other sub-systems from NameNode.init (e.g. Trash), albeit with better documentation. As before there is an api: NameNode.getNameNodeAddress() to get hold of it programmatically. Thanks!

          Show
          Arun C Murthy added a comment - Here is an updated patch to reflect changes to trunk... Konstantin: Appreciate your review. I've had to leave in the code to modify the 'conf' since it is passed to other sub-systems from NameNode.init (e.g. Trash), albeit with better documentation. As before there is an api: NameNode.getNameNodeAddress() to get hold of it programmatically. Thanks!
          Arun C Murthy made changes -
          Attachment HADOOP-1085_20070326_2.patch [ 12354241 ]
          Hide
          Nigel Daley added a comment -

          +1

          I've tested this once on Solaris, Windows, and Linx. This should stabalize our unit tests (time will tell). I think this is a critical patch that we should apply as soon as possible.

          Once it is applied, I will re-enable our patch builds.

          Show
          Nigel Daley added a comment - +1 I've tested this once on Solaris, Windows, and Linx. This should stabalize our unit tests (time will tell). I think this is a critical patch that we should apply as soon as possible. Once it is applied, I will re-enable our patch builds.
          Arun C Murthy made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Doug Cutting added a comment -

          I just committed this. Thanks, Arun!

          Show
          Doug Cutting added a comment - I just committed this. Thanks, Arun!
          Doug Cutting made changes -
          Resolution Fixed [ 1 ]
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hide
          Hadoop QA added a comment -
          Show
          Hadoop QA added a comment - Integrated in Hadoop-Nightly #37 (See http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Nightly/37/ )
          Doug Cutting made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Arun C Murthy
              Reporter:
              Arun C Murthy
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development