Hadoop YARN
  1. Hadoop YARN
  2. YARN-178

Fix custom ProcessTree instance creation

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 3.0.0, 0.23.5
    • Fix Version/s: 3.0.0, 2.0.3-alpha, 0.23.5
    • Component/s: None
    • Labels:
      None

      Description

      1. In current pluggable resourcecalculatorprocesstree is not passed root process id to custom implementation making it unusable.

      2. pstree do not extend Configured as it should

      Added constructor with pid argument with testsuite. Also added test that pstree is correctly configured.

      1. pstree-instance2.txt
        6 kB
        Radim Kolar
      2. pstree-instance.txt
        6 kB
        Radim Kolar

        Activity

        Hide
        Robert Joseph Evans added a comment -

        I have one small issue with the patch. In RefelctionUtils.newInstance when it catches an exception it wraps it in a RuntimeException and then throws it, this patch will instead output a small log message. I would prefer to see it do the same thing as reflectionUtils for compatibility reasons. Other then that the patch looks good to me.

        Show
        Robert Joseph Evans added a comment - I have one small issue with the patch. In RefelctionUtils.newInstance when it catches an exception it wraps it in a RuntimeException and then throws it, this patch will instead output a small log message. I would prefer to see it do the same thing as reflectionUtils for compatibility reasons. Other then that the patch looks good to me.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12550307/pstree-instance.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-YARN-Build/110//testReport/
        Console output: https://builds.apache.org/job/PreCommit-YARN-Build/110//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/12550307/pstree-instance.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/110//testReport/ Console output: https://builds.apache.org/job/PreCommit-YARN-Build/110//console This message is automatically generated.
        Hide
        Robert Joseph Evans added a comment -

        Oh I didn't notice this before, but the test file has '\t' tab characters in it. Could you reformat it to remove the tabs and have it correspond to the two spaces per indentation?

        Show
        Robert Joseph Evans added a comment - Oh I didn't notice this before, but the test file has '\t' tab characters in it. Could you reformat it to remove the tabs and have it correspond to the two spaces per indentation?
        Hide
        Radim Kolar added a comment -

        reformated tabs to spaces, rethrow an exception during instance creation

        Show
        Radim Kolar added a comment - reformated tabs to spaces, rethrow an exception during instance creation
        Hide
        Radim Kolar added a comment -

        i need this to be merged into branch-0.23 as well

        Show
        Radim Kolar added a comment - i need this to be merged into branch-0.23 as well
        Hide
        Bikas Saha added a comment -

        The change looks reasonable. What is not clear is why this is being made? Nothing seems to use the pid in the base class nor is there any other code that uses it.

        Show
        Bikas Saha added a comment - The change looks reasonable. What is not clear is why this is being made? Nothing seems to use the pid in the base class nor is there any other code that uses it.
        Hide
        Radim Kolar added a comment -

        you need to know root pid of process tree to construct it. base class is not using it because its abstract

        Show
        Radim Kolar added a comment - you need to know root pid of process tree to construct it. base class is not using it because its abstract
        Hide
        Bikas Saha added a comment -

        Then why add it to the constructor of the abstract base class?

        Show
        Bikas Saha added a comment - Then why add it to the constructor of the abstract base class?
        Hide
        Radim Kolar added a comment -

        you need constructor which is shared by all subclasses. Alternatively, it could be method instead of constructor but because root process pid do not change then constructor is better.

        Show
        Radim Kolar added a comment - you need constructor which is shared by all subclasses. Alternatively, it could be method instead of constructor but because root process pid do not change then constructor is better.
        Hide
        Robert Joseph Evans added a comment -

        I am fine with either setting it through a contructor or through a setter method. I agree with Radim that the constructor makes it more obvious that it is not supposed to change, but other places tend to use setter methods instead, like with Configuration which is also not supposed to change.

        Show
        Robert Joseph Evans added a comment - I am fine with either setting it through a contructor or through a setter method. I agree with Radim that the constructor makes it more obvious that it is not supposed to change, but other places tend to use setter methods instead, like with Configuration which is also not supposed to change.
        Hide
        Radim Kolar added a comment -

        let patch be as it is. current code is using constructor for linux variant.

        Show
        Radim Kolar added a comment - let patch be as it is. current code is using constructor for linux variant.
        Hide
        Robert Joseph Evans added a comment -

        Makes since. Bikas, unless you have a strong objection I will check this in this afternoon.

        Show
        Robert Joseph Evans added a comment - Makes since. Bikas, unless you have a strong objection I will check this in this afternoon.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12550313/pstree-instance2.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-YARN-Build/122//testReport/
        Console output: https://builds.apache.org/job/PreCommit-YARN-Build/122//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/12550313/pstree-instance2.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/122//testReport/ Console output: https://builds.apache.org/job/PreCommit-YARN-Build/122//console This message is automatically generated.
        Hide
        Bikas Saha added a comment -

        Sure. Please go ahead.

        Show
        Bikas Saha added a comment - Sure. Please go ahead.
        Hide
        Robert Joseph Evans added a comment -

        Sorry I got distracted by other things. Checking this in now.

        Show
        Robert Joseph Evans added a comment - Sorry I got distracted by other things. Checking this in now.
        Hide
        Robert Joseph Evans added a comment -

        Thanks Radim,

        I put this into trunk, branch-2, and branch-0.23

        Show
        Robert Joseph Evans added a comment - Thanks Radim, I put this into trunk, branch-2, and branch-0.23
        Hide
        Hudson added a comment -

        Integrated in Hadoop-trunk-Commit #2921 (See https://builds.apache.org/job/Hadoop-trunk-Commit/2921/)
        YARN-178. Fix custom ProcessTree instance creation (Radim Kolar via bobby) (Revision 1401698)

        Result = SUCCESS
        bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1401698
        Files :

        • /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ResourceCalculatorProcessTree.java
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/TestResourceCalculatorProcessTree.java
        Show
        Hudson added a comment - Integrated in Hadoop-trunk-Commit #2921 (See https://builds.apache.org/job/Hadoop-trunk-Commit/2921/ ) YARN-178 . Fix custom ProcessTree instance creation (Radim Kolar via bobby) (Revision 1401698) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1401698 Files : /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ResourceCalculatorProcessTree.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/TestResourceCalculatorProcessTree.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Yarn-trunk #16 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/16/)
        YARN-178. Fix custom ProcessTree instance creation (Radim Kolar via bobby) (Revision 1401698)

        Result = SUCCESS
        bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1401698
        Files :

        • /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ResourceCalculatorProcessTree.java
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/TestResourceCalculatorProcessTree.java
        Show
        Hudson added a comment - Integrated in Hadoop-Yarn-trunk #16 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/16/ ) YARN-178 . Fix custom ProcessTree instance creation (Radim Kolar via bobby) (Revision 1401698) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1401698 Files : /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ResourceCalculatorProcessTree.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/TestResourceCalculatorProcessTree.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-0.23-Build #415 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/415/)
        YARN-178. Fix custom ProcessTree instance creation (Radim Kolar via bobby) (Revision 1401701)

        Result = SUCCESS
        bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1401701
        Files :

        • /hadoop/common/branches/branch-0.23/hadoop-yarn-project/CHANGES.txt
        • /hadoop/common/branches/branch-0.23/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java
        • /hadoop/common/branches/branch-0.23/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ResourceCalculatorProcessTree.java
        • /hadoop/common/branches/branch-0.23/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/TestResourceCalculatorProcessTree.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #415 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/415/ ) YARN-178 . Fix custom ProcessTree instance creation (Radim Kolar via bobby) (Revision 1401701) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1401701 Files : /hadoop/common/branches/branch-0.23/hadoop-yarn-project/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java /hadoop/common/branches/branch-0.23/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ResourceCalculatorProcessTree.java /hadoop/common/branches/branch-0.23/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/TestResourceCalculatorProcessTree.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #1206 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1206/)
        YARN-178. Fix custom ProcessTree instance creation (Radim Kolar via bobby) (Revision 1401698)

        Result = SUCCESS
        bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1401698
        Files :

        • /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ResourceCalculatorProcessTree.java
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/TestResourceCalculatorProcessTree.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1206 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1206/ ) YARN-178 . Fix custom ProcessTree instance creation (Radim Kolar via bobby) (Revision 1401698) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1401698 Files : /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ResourceCalculatorProcessTree.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/TestResourceCalculatorProcessTree.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #1236 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1236/)
        YARN-178. Fix custom ProcessTree instance creation (Radim Kolar via bobby) (Revision 1401698)

        Result = FAILURE
        bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1401698
        Files :

        • /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ResourceCalculatorProcessTree.java
        • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/TestResourceCalculatorProcessTree.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1236 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1236/ ) YARN-178 . Fix custom ProcessTree instance creation (Radim Kolar via bobby) (Revision 1401698) Result = FAILURE bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1401698 Files : /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ProcfsBasedProcessTree.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ResourceCalculatorProcessTree.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/util/TestResourceCalculatorProcessTree.java

          People

          • Assignee:
            Radim Kolar
            Reporter:
            Radim Kolar
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development