Uploaded image for project: '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
    • Status: Resolved
    • Priority: 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
        hudson Hudson added a comment -

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

        Show
        hudson Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #643 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-trunk/643/ )
        Hide
        hudson 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 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 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 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
        tlipcon Todd Lipcon added a comment -

        Committed to 22 and trunk, thanks Eli and Ravi.

        Show
        tlipcon Todd Lipcon added a comment - Committed to 22 and trunk, thanks Eli and Ravi.
        Hide
        ravidotg 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
        ravidotg 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
        eli 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 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
        tlipcon 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
        tlipcon 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
        ravidotg 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
        ravidotg 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
        eli Eli Collins added a comment -

        +1 change looks good

        Show
        eli Eli Collins added a comment - +1 change looks good
        Hide
        tlipcon 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
        tlipcon 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
        tlipcon 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
        tlipcon 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
        ravidotg Ravi Gummadi added a comment -

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

        Show
        ravidotg Ravi Gummadi added a comment - Tasks will pass. The issue is just the error message saying ulimit failed to set -1 as limit.
        Hide
        vinodkv 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
        vinodkv 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
        ravidotg Ravi Gummadi added a comment -

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

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development