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

Process tree clean up of exceeding memory limit tasks.

    Details

    • Type: Task Task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: test
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      1. Submit a job which would spawn child processes and each of the child processes exceeds the memory limits. Let the job complete . Check if all the child processes are killed, the overall job should fail.

      2. Submit a job which would spawn child processes and each of the child processes exceeds the memory limits. Kill/fail the job while in progress. Check if all the child processes are killed.

      1. memorylimittask_1710.patch
        15 kB
        Vinay Kumar Thota
      2. memorylimittask_1710.patch
        13 kB
        Vinay Kumar Thota
      3. memorylimittask_1710.patch
        13 kB
        Vinay Kumar Thota
      4. memorylimittask_1710.patch
        13 kB
        Vinay Kumar Thota
      5. MAPREDUCE-1710.patch
        13 kB
        Vinay Kumar Thota
      6. ASF.LICENSE.NOT.GRANTED--memorylimittask_1710.patch
        17 kB
        Vinay Kumar Thota
      7. 1710-ydist_security.patch
        13 kB
        Vinay Kumar Thota
      8. 1710-ydist_security.patch
        13 kB
        Vinay Kumar Thota
      9. 1710-ydist_security.patch
        13 kB
        Vinay Kumar Thota

        Issue Links

          Activity

          Hide
          Vinay Kumar Thota added a comment -

          Please review it and let me know your comments.

          Show
          Vinay Kumar Thota added a comment - Please review it and let me know your comments.
          Hide
          Iyappan Srinivasan added a comment -

          Code looks good. Some comments.

          1) Check if all import statements make sense . - NetworkedJob is not required. Check for all imports.

          2) There should be space between parameters in methods like in this : UtilsForSystemTests.restartCluster(cluster,prop); Check this for throughout testcase.

          3) resetClusterWithNewConfigMapred seems a better name than resetCluster, becuse users can understand the intent of this feature just by reading the name rather than seeing the javadocs. The same goes with resetCluster. It can be resetClusterWithOldConfig.

          4) All input, output directory creation and deletion should be done in BeforeClass and AfterClass. This to avoid testcase assuming that a directory is already present. and a testacse cleaning up all directories it created before exiting, even if it failed inteh middle.

          5) if (!tttInfo.isTaskCleanupTask()) - Even setup task could be checked to avoid it and made fully sure its only mapper.

          Show
          Iyappan Srinivasan added a comment - Code looks good. Some comments. 1) Check if all import statements make sense . - NetworkedJob is not required. Check for all imports. 2) There should be space between parameters in methods like in this : UtilsForSystemTests.restartCluster(cluster,prop); Check this for throughout testcase. 3) resetClusterWithNewConfigMapred seems a better name than resetCluster, becuse users can understand the intent of this feature just by reading the name rather than seeing the javadocs. The same goes with resetCluster. It can be resetClusterWithOldConfig. 4) All input, output directory creation and deletion should be done in BeforeClass and AfterClass. This to avoid testcase assuming that a directory is already present. and a testacse cleaning up all directories it created before exiting, even if it failed inteh middle. 5) if (!tttInfo.isTaskCleanupTask()) - Even setup task could be checked to avoid it and made fully sure its only mapper.
          Hide
          Vinay Kumar Thota added a comment -

          3) resetClusterWithNewConfigMapred seems a better name than resetCluster, becuse users can understand the intent of this feature just by reading the name rather than seeing the javadocs. The same goes with resetCluster. It can be resetClusterWithOldConfig.

          [Vinay] : Even I agreed your point and also I thought of defining same kind of method names while developing,however I felt that restartCluster and resetCluster would be fine and there won't be a lengthy method name. Any how user can understand the behavior of method by reading the java docs. So I felt that there is not importance of putting lengthy method name.

          5) if (!tttInfo.isTaskCleanupTask()) - Even setup task could be checked to avoid it and made fully sure its only mapper.

          [Vinay]: I didn't see any method for specific to setup cleanup in ttInfo. the method isTaskCleanupTask might be checking for both.

          Apart from above two comments, I have addressed all other comments and attached the latest patch.

          Show
          Vinay Kumar Thota added a comment - 3) resetClusterWithNewConfigMapred seems a better name than resetCluster, becuse users can understand the intent of this feature just by reading the name rather than seeing the javadocs. The same goes with resetCluster. It can be resetClusterWithOldConfig. [Vinay] : Even I agreed your point and also I thought of defining same kind of method names while developing,however I felt that restartCluster and resetCluster would be fine and there won't be a lengthy method name. Any how user can understand the behavior of method by reading the java docs. So I felt that there is not importance of putting lengthy method name. 5) if (!tttInfo.isTaskCleanupTask()) - Even setup task could be checked to avoid it and made fully sure its only mapper. [Vinay] : I didn't see any method for specific to setup cleanup in ttInfo. the method isTaskCleanupTask might be checking for both. Apart from above two comments, I have addressed all other comments and attached the latest patch.
          Hide
          Iyappan Srinivasan added a comment -

          Since this contains only testcase now and no framework related changes, I have checked the latest patch of testcase and found it good.
          +1

          Show
          Iyappan Srinivasan added a comment - Since this contains only testcase now and no framework related changes, I have checked the latest patch of testcase and found it good. +1
          Hide
          Vinay Kumar Thota added a comment -

          It requires the GenerateTaskChildProcess,so its a depend on 1693 patch and also depend on utility method patch.

          Show
          Vinay Kumar Thota added a comment - It requires the GenerateTaskChildProcess,so its a depend on 1693 patch and also depend on utility method patch.
          Hide
          Vinay Kumar Thota added a comment -

          method signatures changed in Utility class based on Iyappan review comments and and it effects to this patch.So uploading the new patch for it.

          Show
          Vinay Kumar Thota added a comment - method signatures changed in Utility class based on Iyappan review comments and and it effects to this patch.So uploading the new patch for it.
          Hide
          Vinay Kumar Thota added a comment -

          New patch for the tests due to pushconfig utility wrapper changes.

          Show
          Vinay Kumar Thota added a comment - New patch for the tests due to pushconfig utility wrapper changes.
          Hide
          Vinay Kumar Thota added a comment -

          Patch for yahoo distribution security branch.

          Show
          Vinay Kumar Thota added a comment - Patch for yahoo distribution security branch.
          Hide
          Vinay Kumar Thota added a comment -

          Uploading latest patch for Yahoo distribution security branch and this patch requires the MAPREDUCE-1693 and MAPREDUCE-1713.

          Show
          Vinay Kumar Thota added a comment - Uploading latest patch for Yahoo distribution security branch and this patch requires the MAPREDUCE-1693 and MAPREDUCE-1713 .
          Hide
          Vinay Kumar Thota added a comment -

          MAPREDUCE-1713 patch affects this patch because of dependency.So, uploading the new patch.

          Show
          Vinay Kumar Thota added a comment - MAPREDUCE-1713 patch affects this patch because of dependency.So, uploading the new patch.
          Hide
          Vinay Kumar Thota added a comment -

          Patch for trunk.

          Show
          Vinay Kumar Thota added a comment - Patch for trunk.
          Hide
          Konstantin Boudnik added a comment -

          +1 patch looks good. Of you go for the verification.

          Show
          Konstantin Boudnik added a comment - +1 patch looks good. Of you go for the verification.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12449101/MAPREDUCE-1710.patch
          against trunk revision 962682.

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

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

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

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

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

          -1 core tests. The patch failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/596/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/596/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/596/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/596/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/12449101/MAPREDUCE-1710.patch against trunk revision 962682. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/596/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/596/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/596/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/596/console This message is automatically generated.
          Hide
          Vinay Kumar Thota added a comment -

          There are two failures that are not related to this patch.

          Show
          Vinay Kumar Thota added a comment - There are two failures that are not related to this patch.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12449101/MAPREDUCE-1710.patch
          against trunk revision 1075216.

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

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

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

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

          +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 core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/84//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/84//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/84//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/12449101/MAPREDUCE-1710.patch against trunk revision 1075216. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +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 core unit tests. -1 contrib tests. The patch failed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/84//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/84//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/84//console This message is automatically generated.
          Hide
          Arun C Murthy added a comment -

          Sorry to come in late, the patch has gone stale. Can you please rebase? Thanks.

          Given this is not an issue with MRv2 should we still commit this? I'm happy to, but not sure it's useful. Thanks.

          Show
          Arun C Murthy added a comment - Sorry to come in late, the patch has gone stale. Can you please rebase? Thanks. Given this is not an issue with MRv2 should we still commit this? I'm happy to, but not sure it's useful. Thanks.

            People

            • Assignee:
              Vinay Kumar Thota
              Reporter:
              Vinay Kumar Thota
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:

                Development