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

OutputCommitter should have an abortJob method

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.21.0
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Release Note:
      Hide
      Introduced abortJob() method in OutputCommitter which will be invoked when the job fails or is killed. By default it invokes OutputCommitter.cleanupJob(). Deprecated OutputCommitter.cleanupJob() and introduced OutputCommitter.commitJob() method which will be invoked for successful jobs. Also a _SUCCESS file is created in the output folder for successful jobs. A configuration parameter mapreduce.fileoutputcommitter.marksuccessfuljobs can be set to false to disable creation of _SUCCESS file, or to true to enable creation of the _SUCCESS file.
      Show
      Introduced abortJob() method in OutputCommitter which will be invoked when the job fails or is killed. By default it invokes OutputCommitter.cleanupJob(). Deprecated OutputCommitter.cleanupJob() and introduced OutputCommitter.commitJob() method which will be invoked for successful jobs. Also a _SUCCESS file is created in the output folder for successful jobs. A configuration parameter mapreduce.fileoutputcommitter.marksuccessfuljobs can be set to false to disable creation of _SUCCESS file, or to true to enable creation of the _SUCCESS file.

      Description

      The OutputCommitter needs an abortJob method to clean up from failed jobs. Currently there is no way to distinguish between failed or succeeded jobs, making it impossible to write output promotion code.

      1. yhadoop20-bug-fix-947.patch
        0.6 kB
        Ravi Gummadi
      2. mr-947-y20-new.patch
        65 kB
        Jothi Padmanabhan
      3. mr-947-y20.patch
        65 kB
        Jothi Padmanabhan
      4. mr-947-y20.patch
        69 kB
        Jothi Padmanabhan
      5. mr-947-trunk-new.patch
        78 kB
        Jothi Padmanabhan
      6. mr-947-trunk-new.patch
        78 kB
        Jothi Padmanabhan
      7. mr-947-trunk.patch
        78 kB
        Jothi Padmanabhan
      8. mr-947-trunk.patch
        78 kB
        Jothi Padmanabhan
      9. mr-947-trunk.patch
        84 kB
        Arun C Murthy
      10. mapred-948-v3.4.patch
        77 kB
        Amar Kamat
      11. mapred-948-v3.2.patch
        26 kB
        Amar Kamat
      12. mapred-948-v3.1.patch
        26 kB
        Amar Kamat
      13. mapred-948-v2.3-branch-0.20.patch
        23 kB
        Amar Kamat
      14. mapred-948-v2.3.patch
        23 kB
        Amar Kamat
      15. mapred-948-v2.1-branch-0.20.patch
        26 kB
        Amar Kamat
      16. mapred-948-v1.7.patch
        51 kB
        Amar Kamat
      17. mapred-948-v1.4.patch
        49 kB
        Amar Kamat
      18. mapred-948-v1.3.patch
        45 kB
        Amar Kamat
      19. mapred-948-v1.2.patch
        20 kB
        Amar Kamat
      20. mapred-948-v1.13-branch-0.20-internal.patch
        23 kB
        Amar Kamat
      21. mapred-948-v1.12-branch-0.20-internal.patch
        31 kB
        Amar Kamat
      22. mapred-948-v1.12.patch
        52 kB
        Amar Kamat

        Issue Links

          Activity

          Hide
          Amar Kamat added a comment -

          Attaching a patch that adds abortJob() to the committer and calls it upon job failure.
          Result of test-patch
          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 7 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
          Testing the patch.

          Show
          Amar Kamat added a comment - Attaching a patch that adds abortJob() to the committer and calls it upon job failure. Result of test-patch [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 7 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. Testing the patch.
          Hide
          Amar Kamat added a comment -

          Attaching a new patch the fixes some testcase failures. The testcases failed as they ignored _logs from the output dir. Modified it to ignore any file starting with _. Result of test-patch
          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 69 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

          Running ant tests.

          Show
          Amar Kamat added a comment - Attaching a new patch the fixes some testcase failures. The testcases failed as they ignored _logs from the output dir. Modified it to ignore any file starting with _. Result of test-patch [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 69 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. Running ant tests.
          Hide
          Jothi Padmanabhan added a comment -
          1. Should not the _DONE directory be created only for successful jobs? In the patch, the default implementation of abortJob calls cleanupJob, so _DONE will be created for failed/killed job also, no?
          2. The newly added file Utils should be called something like OutputFilterUtils or something similar
          3. There are unused imports in the test cases
          4. The variable LOG in TestJobCleanup is not used
          5. Do we really need a delayed setup for this test case? Could not we achieve the same using a blocking mapper and then kill/fail the job? In such case, we may not need to rely on signaling the setup job for completion.
          6. The test case should also test for absence of _DONE folder for failed/killed jobs
          Show
          Jothi Padmanabhan added a comment - Should not the _DONE directory be created only for successful jobs? In the patch, the default implementation of abortJob calls cleanupJob, so _DONE will be created for failed/killed job also, no? The newly added file Utils should be called something like OutputFilterUtils or something similar There are unused imports in the test cases The variable LOG in TestJobCleanup is not used Do we really need a delayed setup for this test case? Could not we achieve the same using a blocking mapper and then kill/fail the job? In such case, we may not need to rely on signaling the setup job for completion. The test case should also test for absence of _DONE folder for failed/killed jobs
          Hide
          Amar Kamat added a comment -

          Attaching a new patch incorporating Jothi's comments. Result of test-patch
          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 70 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

          Testing in progress.

          Show
          Amar Kamat added a comment - Attaching a new patch incorporating Jothi's comments. Result of test-patch [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 70 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. Testing in progress.
          Hide
          Nigel Daley added a comment -

          Can you comment more on what this will do, how it will work?

          Show
          Nigel Daley added a comment - Can you comment more on what this will do, how it will work?
          Hide
          Amar Kamat added a comment -

          This patch adds a abortJob() method to OutputCommitter which will be invoked for failed/killed jobs in job-cleanup. By default (i.e FileOutputCommitter) the abort job will delete the _temporary files in the output folder (same as cleanupJob()) and create a _abort file (empty file) in the output folder. Also this patch modifies the cleanupJob() method of FileOutputCommitter which will be invoked for successful jobs to create a _done file in the output folder.

          Show
          Amar Kamat added a comment - This patch adds a abortJob() method to OutputCommitter which will be invoked for failed/killed jobs in job-cleanup. By default (i.e FileOutputCommitter) the abort job will delete the _temporary files in the output folder (same as cleanupJob()) and create a _abort file (empty file) in the output folder. Also this patch modifies the cleanupJob() method of FileOutputCommitter which will be invoked for successful jobs to create a _done file in the output folder.
          Hide
          Amar Kamat added a comment -

          Food for thought :

          1. For distinction between running job and completed (succeeded/failed/killed) jobs we can extend the task promotion technique to job i.e while the job is running write the output to a temporary/staging area and then upon job completion promote the output to output.dir. So the simple check is
            if (output.dir exists) {
              job is complete
            } else {
              job is incomplete
            }
            

            One small addition to this is to serialize the job status object to the final output dir so that the clients can simply use the output.dir to get the job status offline.

          1. Having the framework do if-else for successful/unsuccessful job is probably not the right direction. I think the framework should simply leave it to the uses to implement it via OutputCommitter. Hence I think the framework should simply invoke committer.cleanupJob(). FileOutputCommitter should create
            1. _success for successful jobs
            2. _killed for killed jobs
            3. _failed for failed jobs

          The only catch is that the OutputCommitter should know the job state (successful/killed/failed etc).

          Thoughts?

          Show
          Amar Kamat added a comment - Food for thought : For distinction between running job and completed (succeeded/failed/killed) jobs we can extend the task promotion technique to job i.e while the job is running write the output to a temporary/staging area and then upon job completion promote the output to output.dir. So the simple check is if (output.dir exists) { job is complete } else { job is incomplete } One small addition to this is to serialize the job status object to the final output dir so that the clients can simply use the output.dir to get the job status offline. Having the framework do if-else for successful/unsuccessful job is probably not the right direction. I think the framework should simply leave it to the uses to implement it via OutputCommitter. Hence I think the framework should simply invoke committer.cleanupJob() . FileOutputCommitter should create _success for successful jobs _killed for killed jobs _failed for failed jobs The only catch is that the OutputCommitter should know the job state (successful/killed/failed etc). Thoughts?
          Hide
          Hemanth Yamijala added a comment -

          I had a discussion with Amar on the last comment. It appears that some of us had considered the option of changing the task promotion technique and decided against it, as it would be an incompatible change. The same applies for changing the signature of cleanupJob. From that perspective, maybe we should stay the original course of this JIRA.

          However, it may be OK to consider whether we want to future-proof ourselves a little bit and think about ways by which users can distinguish failed and killed jobs in addition to successful jobs. This can possibly be done in a backward compatible manner by introducing a parameter to the abortJob. I am not sure if the use case exists for this requirement, but if we can think of one, it may be better to fix it now rather than latter. Thoughts ?

          Show
          Hemanth Yamijala added a comment - I had a discussion with Amar on the last comment. It appears that some of us had considered the option of changing the task promotion technique and decided against it, as it would be an incompatible change. The same applies for changing the signature of cleanupJob. From that perspective, maybe we should stay the original course of this JIRA. However, it may be OK to consider whether we want to future-proof ourselves a little bit and think about ways by which users can distinguish failed and killed jobs in addition to successful jobs. This can possibly be done in a backward compatible manner by introducing a parameter to the abortJob. I am not sure if the use case exists for this requirement, but if we can think of one, it may be better to fix it now rather than latter. Thoughts ?
          Hide
          Amar Kamat added a comment -

          Attaching a new patch that adds a new method abortJob(JobContext context, boolean failed) to OutputCommitter. Adding failed for failed jobs and __killed for killed jobs in FileOutputCommitter. test-patch passed on my box. Running ant tests.

          Show
          Amar Kamat added a comment - Attaching a new patch that adds a new method abortJob(JobContext context, boolean failed) to OutputCommitter. Adding failed for failed jobs and __killed for killed jobs in FileOutputCommitter. test-patch passed on my box. Running ant tests.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12421502/mapred-948-v1.7.patch
          against trunk revision 819740.

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

          +1 tests included. The patch appears to include 75 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-h3.grid.sp2.yahoo.net/64/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/64/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/64/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/64/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/12421502/mapred-948-v1.7.patch against trunk revision 819740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 75 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-h3.grid.sp2.yahoo.net/64/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/64/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/64/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/64/console This message is automatically generated.
          Hide
          Amar Kamat added a comment -

          Attaching a new patch that fixes the failing test cases. test-patch and ant-test passes.

          Show
          Amar Kamat added a comment - Attaching a new patch that fixes the failing test cases. test-patch and ant-test passes.
          Hide
          Amar Kamat added a comment -

          Attaching an example patch for branch 20 not to be committed.

          Show
          Amar Kamat added a comment - Attaching an example patch for branch 20 not to be committed.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12421567/mapred-948-v1.12-branch-0.20-internal.patch
          against trunk revision 819740.

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/147/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/12421567/mapred-948-v1.12-branch-0.20-internal.patch against trunk revision 819740. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 51 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/147/console This message is automatically generated.
          Hide
          Amar Kamat added a comment -

          Attaching an example patch for branch 20.
          Changes in FileOutputCommitter :

          1. creates _success for successful and nothing for failed/killed jobs
          2. uses a param mapred.fileoutputcommitter.marksuccessfuljobs to create _success file.
          Show
          Amar Kamat added a comment - Attaching an example patch for branch 20. Changes in FileOutputCommitter : creates _success for successful and nothing for failed/killed jobs uses a param mapred.fileoutputcommitter.marksuccessfuljobs to create _success file.
          Hide
          Nigel Daley added a comment -

          Why not just one config param to turn all of this on/off?

          Show
          Nigel Daley added a comment - Why not just one config param to turn all of this on/off?
          Hide
          Amar Kamat added a comment -

          Adding more details on the patch

          1. OutputCommitter.abortJob() will be called for failed and killed jobs. Whether the job has failed or was killed is passed as a boolean paramater named failed in abortJob().
          2. By default abortJob() in OutputCommitter calls cleanupJob() so that we are backward compatible.
          3. FileOutputCommitter overrides abortJob() to do just the temp dir cleanup
          4. FileOutputCommitter's cleanupJob apart from cleaning up temp dir also creates a _success in the output dir. This is optional and is the default behavior. Creation of _success can be controlled by turning off the config param "mapred.fileoutputcommitter.marksuccessfuljobs".
          Show
          Amar Kamat added a comment - Adding more details on the patch OutputCommitter.abortJob() will be called for failed and killed jobs. Whether the job has failed or was killed is passed as a boolean paramater named failed in abortJob(). By default abortJob() in OutputCommitter calls cleanupJob() so that we are backward compatible. FileOutputCommitter overrides abortJob() to do just the temp dir cleanup FileOutputCommitter's cleanupJob apart from cleaning up temp dir also creates a _success in the output dir. This is optional and is the default behavior. Creation of _success can be controlled by turning off the config param "mapred.fileoutputcommitter.marksuccessfuljobs".
          Hide
          Jothi Padmanabhan added a comment -

          A few comments:

          1. The parameter should read mapreduce.fileoutputcommitter.marksuccessfuljobs
          2. You should declare a static string for this configuraiton and use that in the code, as is being done for other configs
          3. The default value for the config should be false (to be backward compatible)
          4. Do the following methods in FileOutputCommitter need to be static? markSuccessfulOutputDir, cleanup
          5. The JavaDoc for abortJob method should document what the boolean is doing (in the mapreduce.outputcommitter)
          6. Minor Nit. In the test case, the comment for MyMapper (Throws OOM for the first attempt) is wrong, MyMapper throws OOM always
          7. Some lines in test case is exceeding 80 characters
          8. Using a delaysetupcommitter in the test case for checking absence of output directory looks wrong, cleanup is called only if the setup succeeds. Instead the test case should actually use a delayed mapper.
          Show
          Jothi Padmanabhan added a comment - A few comments: The parameter should read mapreduce.fileoutputcommitter.marksuccessfuljobs You should declare a static string for this configuraiton and use that in the code, as is being done for other configs The default value for the config should be false (to be backward compatible) Do the following methods in FileOutputCommitter need to be static? markSuccessfulOutputDir, cleanup The JavaDoc for abortJob method should document what the boolean is doing (in the mapreduce.outputcommitter) Minor Nit. In the test case, the comment for MyMapper (Throws OOM for the first attempt) is wrong, MyMapper throws OOM always Some lines in test case is exceeding 80 characters Using a delaysetupcommitter in the test case for checking absence of output directory looks wrong, cleanup is called only if the setup succeeds. Instead the test case should actually use a delayed mapper.
          Hide
          Jothi Padmanabhan added a comment -

          Just realized that this is a 20 patch. So, please ignore point 2 above.

          Show
          Jothi Padmanabhan added a comment - Just realized that this is a 20 patch. So, please ignore point 2 above.
          Hide
          Amar Kamat added a comment -

          Attaching a patch with Jothi's comments incorporated.

          Show
          Amar Kamat added a comment - Attaching a patch with Jothi's comments incorporated.
          Hide
          Jothi Padmanabhan added a comment -

          Looks good. A few comments:

          1. You have missed changing the parameter name in test/mapred-site.xml, it still reads mapred.fileoutputcommitter.marksuccessfuljobs
          2. There are some unused imports in the test case
          3. In testCustomAbort, should the call for testSuccessfulJob be made with the custom-committer? If not, I do not see any difference between this test and the same test in testDefaultCleanupAndAbort; we might as well remove one of them.
          Show
          Jothi Padmanabhan added a comment - Looks good. A few comments: You have missed changing the parameter name in test/mapred-site.xml, it still reads mapred.fileoutputcommitter.marksuccessfuljobs There are some unused imports in the test case In testCustomAbort, should the call for testSuccessfulJob be made with the custom-committer? If not, I do not see any difference between this test and the same test in testDefaultCleanupAndAbort; we might as well remove one of them.
          Hide
          Amar Kamat added a comment -

          Attaching an example patch with comments incorporated for branch 20 not to be committed. Testing in progress.

          Show
          Amar Kamat added a comment - Attaching an example patch with comments incorporated for branch 20 not to be committed. Testing in progress.
          Hide
          Amar Kamat added a comment -

          Attaching a new patch for trunk. The changes are similar to the ones mentioned in the comment. The only difference is that the creation of _success for successful jobs is by default true for trunk. Result of test-patch

          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 9 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

          Testing the patch.

          Show
          Amar Kamat added a comment - Attaching a new patch for trunk. The changes are similar to the ones mentioned in the comment . The only difference is that the creation of _success for successful jobs is by default true for trunk. Result of test-patch [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 9 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. Testing the patch.
          Hide
          Amar Kamat added a comment -

          TestTaskTrackerMemoryManager (timed out) and TestCopyFiles failed. Rest all tests passed.

          Show
          Amar Kamat added a comment - TestTaskTrackerMemoryManager (timed out) and TestCopyFiles failed. Rest all tests passed.
          Hide
          Arun C Murthy added a comment -

          Some comments:

          1. I wish OutputCommitter's methods were called commit and abort rather than the current apis of cleanupJob and abortJob... sigh!
          2. OutputCommitter.abortJob should take a JobStatus.State enum rather than a boolean for jobFailed/jobKilled
          3. Similarly Task should just use the existing boolean jobCleanup and add a JobStatus.State field which should be one of SUCCEEDED, FAILED, KILLED
          4. I'd probably set SUCCEEDED_FILE_NAME to be _done or _DONE
          5. We adding a findbugs suppression, I assume this is for the JobContext being from the different package? We should justify them in future.
          6. Default value of mapreduce.fileoutputcommitter.marksuccessfuljobs should be 'true' and we shouldn't put it in mapred-default.xml; it should be a hidden parameter so we can remove this in the next release.
          Show
          Arun C Murthy added a comment - Some comments: I wish OutputCommitter's methods were called commit and abort rather than the current apis of cleanupJob and abortJob ... sigh! OutputCommitter.abortJob should take a JobStatus.State enum rather than a boolean for jobFailed/jobKilled Similarly Task should just use the existing boolean jobCleanup and add a JobStatus.State field which should be one of SUCCEEDED, FAILED, KILLED I'd probably set SUCCEEDED_FILE_NAME to be _done or _DONE We adding a findbugs suppression, I assume this is for the JobContext being from the different package? We should justify them in future. Default value of mapreduce.fileoutputcommitter.marksuccessfuljobs should be 'true' and we shouldn't put it in mapred-default.xml; it should be a hidden parameter so we can remove this in the next release.
          Hide
          Amar Kamat added a comment -

          I wish OutputCommitter's methods were called commit and abort rather than the current apis of cleanupJob and abortJob... sigh!

          This is insync with the apis for tasks i.e abortTask() and cleanupTask()

          OutputCommitter.abortJob should take a JobStatus.State enum rather than a boolean for jobFailed/jobKilled

          We thought of that but then for an unsuccessful job we dont expect many states other than killed or failed. Do have a case in mind where a enum is must?

          I'd probably set SUCCEEDED_FILE_NAME to be _done or _DONE

          Arun, _done is ambiguous (succeeded/failed/killed). _success is precise.

          We adding a findbugs suppression, I assume this is for the JobContext being from the different package? We should justify them in future.

          Yes. The findbugs is confused between the two classes (mapred and mapreduce).

          Default value of mapreduce.fileoutputcommitter.marksuccessfuljobs should be 'true' and we shouldn't put it in mapred-default.xml; it should be a hidden parameter so we can remove this in the next release.

          It is true and its not mentioned in mapred-default. I have made the default value false for testcases so that they remain unchanged.

          Show
          Amar Kamat added a comment - I wish OutputCommitter's methods were called commit and abort rather than the current apis of cleanupJob and abortJob... sigh! This is insync with the apis for tasks i.e abortTask() and cleanupTask() OutputCommitter.abortJob should take a JobStatus.State enum rather than a boolean for jobFailed/jobKilled We thought of that but then for an unsuccessful job we dont expect many states other than killed or failed. Do have a case in mind where a enum is must? I'd probably set SUCCEEDED_FILE_NAME to be _done or _DONE Arun, _done is ambiguous (succeeded/failed/killed). _success is precise. We adding a findbugs suppression, I assume this is for the JobContext being from the different package? We should justify them in future. Yes. The findbugs is confused between the two classes (mapred and mapreduce). Default value of mapreduce.fileoutputcommitter.marksuccessfuljobs should be 'true' and we shouldn't put it in mapred-default.xml; it should be a hidden parameter so we can remove this in the next release. It is true and its not mentioned in mapred-default. I have made the default value false for testcases so that they remain unchanged.
          Hide
          Amar Kamat added a comment -

          This is insync with the apis for tasks i.e abortTask() and cleanupTask()

          For the task its commitTask(), abortTask() and setupTask(). For a job, cleanupJob() already exists and I havent changed that for backward compatibility. May be we can rename cleanupJob() to commitJob() in a new jira.

          Show
          Amar Kamat added a comment - This is insync with the apis for tasks i.e abortTask() and cleanupTask() For the task its commitTask(), abortTask() and setupTask(). For a job, cleanupJob() already exists and I havent changed that for backward compatibility. May be we can rename cleanupJob() to commitJob() in a new jira.
          Hide
          Arun C Murthy added a comment -

          We thought of that but then for an unsuccessful job we dont expect many states other than killed or failed. Do have a case in mind where a enum is must?

          It is definitely better to be more precise with your apis... a boolean to distinguish betweeen failed and killed is weak. Also it will ensure we don't need 2 booleans in Task, just the current jobCleanup and the enum JobStatus.State.

          Arun, _done is ambiguous (succeeded/failed/killed). _success is precise.

          Uh, ok. I'd definitely go for all caps though - _SUCCEEDED or something like that. It has the happy side-effect of appearing at the top of the 'ls' output! smile

          For the task its commitTask(), abortTask() and setupTask(). For a job, cleanupJob() already exists and I havent changed that for backward compatibility. May be we can rename cleanupJob() to commitJob() in a new jira.

          That was when I let out my sigh. We cannot do that now - too late, we just have to live with it.

          Show
          Arun C Murthy added a comment - We thought of that but then for an unsuccessful job we dont expect many states other than killed or failed. Do have a case in mind where a enum is must? It is definitely better to be more precise with your apis... a boolean to distinguish betweeen failed and killed is weak. Also it will ensure we don't need 2 booleans in Task, just the current jobCleanup and the enum JobStatus.State. Arun, _done is ambiguous (succeeded/failed/killed). _success is precise. Uh, ok. I'd definitely go for all caps though - _SUCCEEDED or something like that. It has the happy side-effect of appearing at the top of the 'ls' output! smile For the task its commitTask(), abortTask() and setupTask(). For a job, cleanupJob() already exists and I havent changed that for backward compatibility. May be we can rename cleanupJob() to commitJob() in a new jira. That was when I let out my sigh. We cannot do that now - too late, we just have to live with it.
          Hide
          Devaraj Das added a comment -

          Can't we at least deprecate the cleanupJob now ? And define a new one like commitJob that cleanupJob calls?

          Show
          Devaraj Das added a comment - Can't we at least deprecate the cleanupJob now ? And define a new one like commitJob that cleanupJob calls?
          Hide
          Amar Kamat added a comment -

          Attaching a new patch.

          Show
          Amar Kamat added a comment - Attaching a new 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/12421972/mapred-948-v2.3.patch
          against trunk revision 824750.

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

          +1 tests included. The patch appears to include 9 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/165/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/165/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/165/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/165/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/12421972/mapred-948-v2.3.patch against trunk revision 824750. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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/165/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/165/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/165/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/165/console This message is automatically generated.
          Hide
          Jothi Padmanabhan added a comment -

          In src/java/org/apache/hadoop/mapreduce/lib/output/FileOutputCommitter.java, abortJob is calling cleanupJob instead of cleanup. This causes _SUCCESS files to be created for Killed and Failed Jobs too.

          Show
          Jothi Padmanabhan added a comment - In src/java/org/apache/hadoop/mapreduce/lib/output/FileOutputCommitter.java, abortJob is calling cleanupJob instead of cleanup. This causes _SUCCESS files to be created for Killed and Failed Jobs too.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422069/mapred-948-v3.1.patch
          against trunk revision 825044.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 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/166/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/166/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/166/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/166/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/12422069/mapred-948-v3.1.patch against trunk revision 825044. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 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/166/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/166/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/166/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/166/console This message is automatically generated.
          Hide
          Arun C Murthy added a comment -

          This seems to cause a lot of streaming tests to fail... can you please investigate?

          Show
          Arun C Murthy added a comment - This seems to cause a lot of streaming tests to fail... can you please investigate?
          Hide
          Amar Kamat added a comment -

          Attaching a new patch with a small bug fix. Test-patch passed.

          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 9 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

          Testing in progress.

          Show
          Amar Kamat added a comment - Attaching a new patch with a small bug fix. Test-patch passed. [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 9 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. Testing in progress.
          Hide
          Amar Kamat added a comment -

          TestTaskTrackerMemoryManager FAILED (timeout). Rest all testcases passes.

          Show
          Amar Kamat added a comment - TestTaskTrackerMemoryManager FAILED (timeout). Rest all testcases passes.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422145/mapred-948-v3.2.patch
          against trunk revision 825083.

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

          +1 tests included. The patch appears to include 9 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 passed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/75/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/75/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/75/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/75/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/12422145/mapred-948-v3.2.patch against trunk revision 825083. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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 passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/75/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/75/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/75/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/75/console This message is automatically generated.
          Hide
          Amar Kamat added a comment -

          All tests passes on my box. Not sure why they are failing with hudson.

          Show
          Amar Kamat added a comment - All tests passes on my box. Not sure why they are failing with hudson.
          Hide
          Arun C Murthy added a comment -

          After thinking some more, I think we should take Devaraj's suggestion and deprecate OutputCommitter.cleanupJob and add new methods: OutputCommitter.

          {commitJob|abortJob}, this is as good a time as any to fix it. For BC both the new apis should call the cleanupJob, thus they need to be concrete (not abstract) methods.

          Some more comments:
          # Please remove FileOutputCommitter.setOutputDirMarking since we are making it a hidden feature for 0.22, also make FileOutputCommitter.SUCCESSFUL_JOB_OUTPUT_DIR_MARKER package-private for similar reasons.
          # Ditto for o.a.h.mapred.FileOutputCommitter
          # o.a.h.mapred.FileOutputCommitter.SUCCEEDED_FILE_NAME is public while o.a.h.mapreduce.FileOutputCommiter.SUCCEEDED_FILE_NAME is protected. Shouldn't both be package-private or public?
          # I think I've changed my mind about o.a.h.mapred.OutputCommitter.abortJob - it should take an int rather than o.a.h.mapreduce.JobStatus.State. It is odd that o.a.h.mapred.* depends on o.a.h.mapreduce.*
          # o.a.h.{mapred|mapreduce}.OutputFormat.{commitJob|abortJob}

          should assert that the job's state is SUCCESS or FAILED/KILLED appropriately in the javadoc (I see that Task.java checks it appropriately).

          1. I don't think we should specify mapreduce.fileoutputcommitter.marksuccessfuljobs to be 'false' in src/test/mapred-site.xml. Rather, we should hunt down and fix test-cases which fail appropriately! (via PathFilter to 'ls' etc.)
          Show
          Arun C Murthy added a comment - After thinking some more, I think we should take Devaraj's suggestion and deprecate OutputCommitter.cleanupJob and add new methods: OutputCommitter. {commitJob|abortJob}, this is as good a time as any to fix it. For BC both the new apis should call the cleanupJob, thus they need to be concrete (not abstract) methods. Some more comments: # Please remove FileOutputCommitter.setOutputDirMarking since we are making it a hidden feature for 0.22, also make FileOutputCommitter.SUCCESSFUL_JOB_OUTPUT_DIR_MARKER package-private for similar reasons. # Ditto for o.a.h.mapred.FileOutputCommitter # o.a.h.mapred.FileOutputCommitter.SUCCEEDED_FILE_NAME is public while o.a.h.mapreduce.FileOutputCommiter.SUCCEEDED_FILE_NAME is protected. Shouldn't both be package-private or public? # I think I've changed my mind about o.a.h.mapred.OutputCommitter.abortJob - it should take an int rather than o.a.h.mapreduce.JobStatus.State. It is odd that o.a.h.mapred.* depends on o.a.h.mapreduce.* # o.a.h.{mapred|mapreduce}.OutputFormat.{commitJob|abortJob} should assert that the job's state is SUCCESS or FAILED/KILLED appropriately in the javadoc (I see that Task.java checks it appropriately). I don't think we should specify mapreduce.fileoutputcommitter.marksuccessfuljobs to be 'false' in src/test/mapred-site.xml. Rather, we should hunt down and fix test-cases which fail appropriately! (via PathFilter to 'ls' etc.)
          Hide
          Amar Kamat added a comment -

          Attaching a patch for trunk that incorporate Arun's comments. Result of
          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 145 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

          All core and contrib tests passed.

          The problem with the failing streaming test cases was

          1. contrib test doesnt respect src/test/mapred-site.xml
          2. streaming testcase expect the build/contrib/streaming/test/data/ folder to have empty out folder. Fixed the tests to not create _SUCCESS.
          Show
          Amar Kamat added a comment - Attaching a patch for trunk that incorporate Arun's comments. Result of [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 145 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. All core and contrib tests passed. The problem with the failing streaming test cases was contrib test doesnt respect src/test/mapred-site.xml streaming testcase expect the build/contrib/streaming/test/data/ folder to have empty out folder. Fixed the tests to not create _SUCCESS.
          Hide
          Jothi Padmanabhan added a comment -

          Made modifications to Amar's patch to do:
          1. Make cleanupJob non abstract with an empty implementation
          2. Modified streaming tests to recursive delete the output directories so that we do not need to explicitly turn off the configuration to mark successful job completion.

          Show
          Jothi Padmanabhan added a comment - Made modifications to Amar's patch to do: 1. Make cleanupJob non abstract with an empty implementation 2. Modified streaming tests to recursive delete the output directories so that we do not need to explicitly turn off the configuration to mark successful job completion.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422546/mr-947-trunk.patch
          against trunk revision 826635.

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

          +1 tests included. The patch appears to include 126 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 passed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/185/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/185/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/185/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/185/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/12422546/mr-947-trunk.patch against trunk revision 826635. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 126 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/185/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/185/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/185/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/185/console This message is automatically generated.
          Hide
          Arun C Murthy added a comment -

          Hmm... o.a.h.mapreduce.OutputCommitter.abortJob calls commitJob by default, shouldn't it call cleanupJob instead?

          Show
          Arun C Murthy added a comment - Hmm... o.a.h.mapreduce.OutputCommitter.abortJob calls commitJob by default, shouldn't it call cleanupJob instead?
          Hide
          Arun C Murthy added a comment -

          Hmm... o.a.h.mapreduce.OutputCommitter.abortJob calls commitJob by default, shouldn't it call cleanupJob instead?

          Ditto for o.a.h.mapred.OutputCommitter.abortJob.


          @deprecated javadoc for o.a.h.mapred.OutputCommitter.cleanupJob should point to both new apis like so:

             * @deprecated Use {@link #commitJob(JobContext)} or {@link #abortJob(JobContext, int)} instead.
          
          Show
          Arun C Murthy added a comment - Hmm... o.a.h.mapreduce.OutputCommitter.abortJob calls commitJob by default, shouldn't it call cleanupJob instead? Ditto for o.a.h.mapred.OutputCommitter.abortJob. @deprecated javadoc for o.a.h.mapred.OutputCommitter.cleanupJob should point to both new apis like so: * @deprecated Use {@link #commitJob(JobContext)} or {@link #abortJob(JobContext, int)} instead.
          Hide
          Jothi Padmanabhan added a comment -

          o.a.h.mapreduce.OutputCommitter.abortJob calls commitJob by default, shouldn't it call cleanupJob instead?

          I am not sure if we want to do this. The user could implement commitJob and not a cleanupJob, in which case abortJob should call commitJob by default, no?

          Show
          Jothi Padmanabhan added a comment - o.a.h.mapreduce.OutputCommitter.abortJob calls commitJob by default, shouldn't it call cleanupJob instead? I am not sure if we want to do this. The user could implement commitJob and not a cleanupJob, in which case abortJob should call commitJob by default, no?
          Hide
          Jothi Padmanabhan added a comment -

          Changes from the prev patch

          1. abortJob calls cleanupJob
          2. Javadoc for cleanup mentions both abort and commit
          3. Modified on sqoop test case to use output filter instead of blanket turning off the configuration for all sqoop tests
          Show
          Jothi Padmanabhan added a comment - Changes from the prev patch abortJob calls cleanupJob Javadoc for cleanup mentions both abort and commit Modified on sqoop test case to use output filter instead of blanket turning off the configuration for all sqoop tests
          Hide
          Arun C Murthy added a comment -

          Jothi, this is looking good...

          I have some minor nits I should have pointed out earlier, my bad - apologies:

          1. FileOutputCommitter shouldn't override deprecated cleanupJob at all.
          2. FileOutputCommitter silently ignores cases where FileOutputFormat.getOutputPath returns null. This isn't new - I just thought we should fix it to log a warning alongwith context (i.e. which function) where it returns null in all cases.
          3. Task.java should just call the final org.apache.hadoop.mapred.OutputCommitter.abortJob(JobContext, State) rather than call
            org.apache.hadoop.mapred.OutputCommitter.abortJob(JobContext, int) after type-conversion from State to int
          4. JobStatus.getOldNewJobRunState can just use org.apache.hadoop.mapreduce.JobStatus.State.getValue(), no?
          5. javadoc for o.a.h.mapred.OutputCommitter.cleanupJob needs a space between the link and instead
          6. javadoc for o.a.h. {mapred|mapreduce}

            .OutputCommitter.abortJob can be improved to use links for the FAILED/KILLED states as

            {@link JobStatus#FAILED}

            etc.

          Show
          Arun C Murthy added a comment - Jothi, this is looking good... I have some minor nits I should have pointed out earlier, my bad - apologies: FileOutputCommitter shouldn't override deprecated cleanupJob at all. FileOutputCommitter silently ignores cases where FileOutputFormat.getOutputPath returns null. This isn't new - I just thought we should fix it to log a warning alongwith context (i.e. which function) where it returns null in all cases. Task.java should just call the final org.apache.hadoop.mapred.OutputCommitter.abortJob(JobContext, State) rather than call org.apache.hadoop.mapred.OutputCommitter.abortJob(JobContext, int) after type-conversion from State to int JobStatus.getOldNewJobRunState can just use org.apache.hadoop.mapreduce.JobStatus.State.getValue(), no? javadoc for o.a.h.mapred.OutputCommitter.cleanupJob needs a space between the link and instead javadoc for o.a.h. {mapred|mapreduce} .OutputCommitter.abortJob can be improved to use links for the FAILED/KILLED states as {@link JobStatus#FAILED} etc.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422651/mr-947-trunk.patch
          against trunk revision 826767.

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

          +1 tests included. The patch appears to include 126 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 passed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/83/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/83/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/83/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/83/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/12422651/mr-947-trunk.patch against trunk revision 826767. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 126 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/83/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/83/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/83/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/83/console This message is automatically generated.
          Hide
          Jothi Padmanabhan added a comment -

          Patch incorporating review comments

          Show
          Jothi Padmanabhan added a comment - Patch incorporating review comments
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422668/mr-947-trunk-new.patch
          against trunk revision 826979.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 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 passed 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/192/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/192/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/192/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/192/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/12422668/mr-947-trunk-new.patch against trunk revision 826979. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 126 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 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 passed 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/192/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/192/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/192/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/192/console This message is automatically generated.
          Hide
          Jothi Padmanabhan added a comment -

          The failed test, TestGridmixSubmission, is unrelated to the patch and the failure is being tracked in MAPREDUCE-1124

          Show
          Jothi Padmanabhan added a comment - The failed test, TestGridmixSubmission, is unrelated to the patch and the failure is being tracked in MAPREDUCE-1124
          Hide
          Jothi Padmanabhan added a comment -

          Patch for the Y!20 distribution

          Show
          Jothi Padmanabhan added a comment - Patch for the Y!20 distribution
          Hide
          Hemanth Yamijala added a comment -

          Jothi, is the -1 on javadoc expected ?

          Show
          Hemanth Yamijala added a comment - Jothi, is the -1 on javadoc expected ?
          Hide
          Arun C Murthy added a comment -

          Jothi, is the -1 on javadoc expected ?

          Jothi - could this be due to the wrong import in o.a.h.mapreduce.OutputCommitter:

          Index: src/java/org/apache/hadoop/mapreduce/OutputCommitter.java
          ===================================================================
          --- src/java/org/apache/hadoop/mapreduce/OutputCommitter.java	(revision 826468)
          +++ src/java/org/apache/hadoop/mapreduce/OutputCommitter.java	(working copy)
          @@ -20,6 +20,8 @@
           
           import java.io.IOException;
           
          +import org.apache.hadoop.mapred.JobStatus;
          +
          
          Show
          Arun C Murthy added a comment - Jothi, is the -1 on javadoc expected ? Jothi - could this be due to the wrong import in o.a.h.mapreduce.OutputCommitter: Index: src/java/org/apache/hadoop/mapreduce/OutputCommitter.java =================================================================== --- src/java/org/apache/hadoop/mapreduce/OutputCommitter.java (revision 826468) +++ src/java/org/apache/hadoop/mapreduce/OutputCommitter.java (working copy) @@ -20,6 +20,8 @@ import java.io.IOException; +import org.apache.hadoop.mapred.JobStatus; +
          Hide
          Arun C Murthy added a comment -

          And the javadoc for abortJob should be:

            /**
             * For aborting an unsuccessful job's output. Note that this is invoked for 
             * jobs with final runstate as {@link JobStatus.State#FAILED} or 
             * {@link JobStatus.State#KILLED}
             *
             * @param jobContext Context of the job whose output is being written.
             * @param state final runstate of the job
             * @throws IOException
             */
            public void abortJob(JobContext jobContext, JobStatus.State state) 
            throws IOException {
          
          Show
          Arun C Murthy added a comment - And the javadoc for abortJob should be: /** * For aborting an unsuccessful job's output. Note that this is invoked for * jobs with final runstate as {@link JobStatus.State#FAILED} or * {@link JobStatus.State#KILLED} * * @param jobContext Context of the job whose output is being written. * @param state final runstate of the job * @throws IOException */ public void abortJob(JobContext jobContext, JobStatus.State state) throws IOException {
          Hide
          Jothi Padmanabhan added a comment -

          Patch that fixes the java doc warning

          Show
          Jothi Padmanabhan added a comment - Patch that fixes the java doc warning
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422763/mr-947-trunk-new.patch
          against trunk revision 827854.

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

          +1 tests included. The patch appears to include 126 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 passed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/86/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/86/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/86/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/86/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/12422763/mr-947-trunk-new.patch against trunk revision 827854. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 126 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/86/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/86/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/86/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/86/console This message is automatically generated.
          Hide
          Jothi Padmanabhan added a comment -

          All tests passed this time as compared to one test, TestGridmixSubmission last time, even though there were no code changes between the two patches. Just confirms the observation in MAPREDUCE-1124 that this failure is sporadic; does not happen all the time.

          Show
          Jothi Padmanabhan added a comment - All tests passed this time as compared to one test, TestGridmixSubmission last time, even though there were no code changes between the two patches. Just confirms the observation in MAPREDUCE-1124 that this failure is sporadic; does not happen all the time.
          Hide
          Jothi Padmanabhan added a comment -

          Patch for the Y20 distribution

          Show
          Jothi Padmanabhan added a comment - Patch for the Y20 distribution
          Hide
          Arun C Murthy added a comment -

          Minor edit to the patch to add @Deprecated annotations to cleanupJob.

          $ ant test-patch
          
               [exec] +1 overall.  
               [exec] 
               [exec]     +1 @author.  The patch does not contain any @author tags.
               [exec] 
               [exec]     +1 tests included.  The patch appears to include 125 new or modified tests.
               [exec] 
               [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
               [exec] 
               [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
               [exec] 
               [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
               [exec] 
               [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
               [exec] 
          
          
          Show
          Arun C Murthy added a comment - Minor edit to the patch to add @Deprecated annotations to cleanupJob. $ ant test-patch [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 125 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec]
          Hide
          Arun C Murthy added a comment -

          I just committed this. Thanks, Amar & Jothi!

          Show
          Arun C Murthy added a comment - I just committed this. Thanks, Amar & Jothi!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #90 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/90/)
          . Added commitJob and abortJob apis to OutputCommitter. Enhanced FileOutputCommitter to create a _SUCCESS file for successful jobs. Contributed by Amar Kamat & Jothi Padmanabhan.

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #90 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/90/ ) . Added commitJob and abortJob apis to OutputCommitter. Enhanced FileOutputCommitter to create a _SUCCESS file for successful jobs. Contributed by Amar Kamat & Jothi Padmanabhan.
          Hide
          Iyappan Srinivasan added a comment -

          +1 from QA for mr-947-y20-new.patch.

          1) When mapreduce.fileoutputcomitter.marksuccessfuljobs parm is set to false

          a) Successful jobs passes normally without creating a _SUCCESS file in the output directory.
          b) Failed jobs fails normally without creating any file in the output directory.

          2) When mapreduce.fileoutputcomitter.marksuccessfuljobs is set to true, then

          a) Successful jobs passes and creates _SUCCESS under output directory.
          b) Failed jobs, which are running midway, does not create _ SUCCESS when killed
          c) Failed jobs, when doing their cleanup does not create _SUCCESS file when killed.
          d) Failed jobs, when doing setup, does not create any output directory and so does not create any file under it.

          3) After JT restart, and When mapreduce.fileoutputcomitter.marksuccessfuljobs is set to true, then

          a) Successful jobs passes and creates _ SUCCESS under output directory.
          b) Failed jobs, which are running midway, does not create _ SUCCESS when killed
          c) Failed jobs, when doing setup, does not create any output directory and so does not create any file under it.

          4) Run 100 jobs and kill/fail some jobs in the middle. The killed/failed ones should fail properly and no string displayed. The successful jobs should display success.

          5) Run randomwriter and sort. Both should work properly.

          Show
          Iyappan Srinivasan added a comment - +1 from QA for mr-947-y20-new.patch. 1) When mapreduce.fileoutputcomitter.marksuccessfuljobs parm is set to false a) Successful jobs passes normally without creating a _SUCCESS file in the output directory. b) Failed jobs fails normally without creating any file in the output directory. 2) When mapreduce.fileoutputcomitter.marksuccessfuljobs is set to true, then a) Successful jobs passes and creates _SUCCESS under output directory. b) Failed jobs, which are running midway, does not create _ SUCCESS when killed c) Failed jobs, when doing their cleanup does not create _SUCCESS file when killed. d) Failed jobs, when doing setup, does not create any output directory and so does not create any file under it. 3) After JT restart, and When mapreduce.fileoutputcomitter.marksuccessfuljobs is set to true, then a) Successful jobs passes and creates _ SUCCESS under output directory. b) Failed jobs, which are running midway, does not create _ SUCCESS when killed c) Failed jobs, when doing setup, does not create any output directory and so does not create any file under it. 4) Run 100 jobs and kill/fail some jobs in the middle. The killed/failed ones should fail properly and no string displayed. The successful jobs should display success. 5) Run randomwriter and sort. Both should work properly.
          Hide
          Jothi Padmanabhan added a comment -

          Patch for the Y! 20 distribution.

          Show
          Jothi Padmanabhan added a comment - Patch for the Y! 20 distribution.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #120 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/120/)

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #120 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/120/ )
          Hide
          Ravi Gummadi added a comment -

          Y! 20 patch has a bug that made TestJobHistory to fail. Patch with the fix for the bug for Y! 20 distribution is attached now.

          Running unit tests with the fix now.

          Show
          Ravi Gummadi added a comment - Y! 20 patch has a bug that made TestJobHistory to fail. Patch with the fix for the bug for Y! 20 distribution is attached now. Running unit tests with the fix now.
          Hide
          Ravi Gummadi added a comment -

          ant test and ant test-patch passed on my local machine with this fix for Y! 20 distribution.

          Show
          Ravi Gummadi added a comment - ant test and ant test-patch passed on my local machine with this fix for Y! 20 distribution.
          Hide
          Hemanth Yamijala added a comment -

          +1 for Ravi's patch.

          Show
          Hemanth Yamijala added a comment - +1 for Ravi's patch.

            People

            • Assignee:
              Amar Kamat
              Reporter:
              Owen O'Malley
            • Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development