Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-1085

For tasks, "ulimit -v -1" is being run when user doesn't specify mapred.child.ulimit

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.21.0, 0.22.0
    • Fix Version/s: 0.22.0
    • Component/s: tasktracker
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      For tasks, "ulimit -v -1" is being run when user doesn't specify mapred.child.ulimit. Taking -1 as default value and using it in building the command is not right.

        Activity

        Hide
        Ravi Gummadi added a comment -

        We can check if the value is positive or not and add the ulimit command only if needed.

        Show
        Ravi Gummadi added a comment - We can check if the value is positive or not and add the ulimit command only if needed.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        This is a regression.

        Before MAPREDUCE-478, if mapred.child.ulimit is not configured in mapred-site.xml, tasks run successfully.
        After MAPREDUCE-478, they fail with error saying invalid options for ulimit (ulimit -v -1).

        Show
        Vinod Kumar Vavilapalli added a comment - This is a regression. Before MAPREDUCE-478 , if mapred.child.ulimit is not configured in mapred-site.xml, tasks run successfully. After MAPREDUCE-478 , they fail with error saying invalid options for ulimit (ulimit -v -1).
        Hide
        Ravi Gummadi added a comment -

        Tasks will pass. The issue is just the error message saying ulimit failed to set -1 as limit.

        Show
        Ravi Gummadi added a comment - Tasks will pass. The issue is just the error message saying ulimit failed to set -1 as limit.
        Hide
        Todd Lipcon added a comment -

        Patch simply sets the ulimit command to null if it's set to <=0

        Unfortunately this is very difficult to get to via a new unit test since it's deep within private methods, but should be straightforward and the code is covered by lots of integration tests.

        Show
        Todd Lipcon added a comment - Patch simply sets the ulimit command to null if it's set to <=0 Unfortunately this is very difficult to get to via a new unit test since it's deep within private methods, but should be straightforward and the code is covered by lots of integration tests.
        Hide
        Todd Lipcon added a comment -

        Tests passed except for a few timeouts which I can't seem to reproduce, and a few failures which are known on trunk.

        Show
        Todd Lipcon added a comment - Tests passed except for a few timeouts which I can't seem to reproduce, and a few failures which are known on trunk.
        Hide
        Eli Collins added a comment -

        +1 change looks good

        Show
        Eli Collins added a comment - +1 change looks good
        Hide
        Ravi Gummadi added a comment -

        Since getVMSetupCmd() is returning null, the caller's code will also needs changes ?
        Caller of getVMSetupCmd() has the following code.

        String setup = getVMSetupCmd();
        setupCmds.add(setup);
        

        setup can be null here.
        Adding null as a command to the list of commands is useless and may cause runtime issues.
        A null check like the following is better.

        if (setup != null) {
          setupCmds.add(setup);
        }
        
        Show
        Ravi Gummadi added a comment - Since getVMSetupCmd() is returning null, the caller's code will also needs changes ? Caller of getVMSetupCmd() has the following code. String setup = getVMSetupCmd(); setupCmds.add(setup); setup can be null here. Adding null as a command to the list of commands is useless and may cause runtime issues. A null check like the following is better. if (setup != null ) { setupCmds.add(setup); }
        Hide
        Todd Lipcon added a comment -

        We already return null from getVMSetupCmd in other cases - take a look at Shell.getUlimitMemoryCommand and you'll see that we return null there for Windows as is.

        Not sure which code you're refering to above with setupCmds.add(setup). It's not in trunk.
        The flow is: launchJvmAndWait -> JvmManager.constructJvmEnv -> JvmEnv data structure -> TaskLog.buildCommandLine where you see the null check:

            if (setup != null && setup.size() > 0) {
              mergedCmd.append(addCommand(setup, false));
              mergedCmd.append(";");
            }
        
        Show
        Todd Lipcon added a comment - We already return null from getVMSetupCmd in other cases - take a look at Shell.getUlimitMemoryCommand and you'll see that we return null there for Windows as is. Not sure which code you're refering to above with setupCmds.add(setup). It's not in trunk. The flow is: launchJvmAndWait -> JvmManager.constructJvmEnv -> JvmEnv data structure -> TaskLog.buildCommandLine where you see the null check: if (setup != null && setup.size() > 0) { mergedCmd.append(addCommand(setup, false )); mergedCmd.append( ";" ); }
        Hide
        Eli Collins added a comment -

        What branch do you see this code in? I don't see the code you're referring to on trunk or 21. There's only one caller of getVMSetupCmd and it's fine with the null return value.

        Show
        Eli Collins added a comment - What branch do you see this code in? I don't see the code you're referring to on trunk or 21. There's only one caller of getVMSetupCmd and it's fine with the null return value.
        Hide
        Ravi Gummadi added a comment -

        I was looking at older branch. Code is different in trunk.

        Fine. Trunk's code is checking for null in both cases of DefaultTaskController and LinuxTaskController. So the code changes of the patch should be fine.

        +1.

        Show
        Ravi Gummadi added a comment - I was looking at older branch. Code is different in trunk. Fine. Trunk's code is checking for null in both cases of DefaultTaskController and LinuxTaskController. So the code changes of the patch should be fine. +1.
        Hide
        Todd Lipcon added a comment -

        Committed to 22 and trunk, thanks Eli and Ravi.

        Show
        Todd Lipcon added a comment - Committed to 22 and trunk, thanks Eli and Ravi.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #578 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/578/)
        MAPREDUCE-1085. For tasks, "ulimit -v -1" is being run when user doesn't specify a ulimit. Contributed by Todd Lipcon

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #578 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/578/ ) MAPREDUCE-1085 . For tasks, "ulimit -v -1" is being run when user doesn't specify a ulimit. Contributed by Todd Lipcon
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-22-branch #33 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-22-branch/33/)

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-22-branch #33 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-22-branch/33/ )
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #643 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-trunk/643/)

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #643 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-trunk/643/ )

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Ravi Gummadi
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development