Hadoop Common
  1. Hadoop Common
  2. HADOOP-2344

Free up the buffers (input and error) while executing a shell command before waiting for it to finish.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.16.0
    • Fix Version/s: 0.16.0
    • Component/s: None
    • Labels:
      None

      Description

      Process.waitFor() should be invoked after freeing up the input and error stream. While fixing https://issues.apache.org/jira/browse/HADOOP-2231 we found that this might be a possible cause.

      1. HADOOP-2231.patch
        8 kB
        Amar Kamat
      2. HADOOP-2344.patch
        11 kB
        Amar Kamat
      3. HADOOP-2344.patch
        13 kB
        Amar Kamat
      4. HADOOP-2344.patch
        13 kB
        Amar Kamat
      5. HADOOP-2344.patch
        23 kB
        Amar Kamat
      6. HADOOP-2344.patch
        29 kB
        Amar Kamat
      7. HADOOP-2344.patch
        29 kB
        Arun C Murthy

        Issue Links

          Activity

          Hide
          Amar Kamat added a comment -

          Submitting a patch for ShellCommand.java and ShellUtil.java.
          Comments?

          Show
          Amar Kamat added a comment - Submitting a patch for ShellCommand.java and ShellUtil.java . 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/12370922/HADOOP-2231.patch
          against trunk revision r600952.

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

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

          javac +1. The applied patch does not generate any new compiler warnings.

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

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

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

          Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1258/testReport/
          Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1258/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1258/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1258/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/12370922/HADOOP-2231.patch against trunk revision r600952. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests +1. The patch passed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1258/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1258/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1258/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1258/console This message is automatically generated.
          Hide
          Doug Cutting added a comment -

          I think you mean to override Thread#run(), not Thread#start(), no?

          Also, do we need both ShellUtil & ShellCommand? It would seem less error prone to have a single utility for executing command lines.

          I think this is a bug worth fixing. In a previous version of DF.java, I recall finding that failing to explicitly close all streams could cause file descriptor leaks. Now it seems that in the current version streams are no longer closed again. And reading process output before checking for exit status also seems prudent.

          Show
          Doug Cutting added a comment - I think you mean to override Thread#run(), not Thread#start(), no? Also, do we need both ShellUtil & ShellCommand? It would seem less error prone to have a single utility for executing command lines. I think this is a bug worth fixing. In a previous version of DF.java, I recall finding that failing to explicitly close all streams could cause file descriptor leaks. Now it seems that in the current version streams are no longer closed again. And reading process output before checking for exit status also seems prudent.
          Hide
          Amar Kamat added a comment -

          This patch now merges the functionalities of ShellCommand and ShellUtil. I have deprecated ShellUtil. Now one can use ShellCommand in the following ways

          • use ShellCommand.ShellCommandExecutor to simply execute a command
          • extend ShellCommand to do some advance level output parsing

          Basically use ShellCommandExecutor

          • if the expected output is small and the requirement is to simply execute the command

          Extending ShellCommand might be useful in cases where there is a need to control

          • input directories
          • command
          • shell environment

          Regarding the "Process.waitFor() hangs" issue, this patch clears the error stream using a separate thread while the input stream is read in the current thread (so that they happen in parallel). Once the streams are cleared, ShellCommand waits for the process to terminate and come out of waitFor(). Finally the process is destroyed closing the input/error streams. As done in ShellUtil, the process is made accessible outside ShellCommand so that the process can be externally terminated.

          Show
          Amar Kamat added a comment - This patch now merges the functionalities of ShellCommand and ShellUtil . I have deprecated ShellUtil . Now one can use ShellCommand in the following ways use ShellCommand.ShellCommandExecutor to simply execute a command extend ShellCommand to do some advance level output parsing Basically use ShellCommandExecutor if the expected output is small and the requirement is to simply execute the command Extending ShellCommand might be useful in cases where there is a need to control input directories command shell environment Regarding the "Process.waitFor() hangs" issue, this patch clears the error stream using a separate thread while the input stream is read in the current thread (so that they happen in parallel). Once the streams are cleared, ShellCommand waits for the process to terminate and come out of waitFor() . Finally the process is destroyed closing the input/error streams. As done in ShellUtil , the process is made accessible outside ShellCommand so that the process can be externally terminated.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12371201/HADOOP-2344.patch
          against trunk revision r601961.

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

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

          javac +1. The applied patch does not generate any new compiler warnings.

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

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

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

          Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1290/testReport/
          Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1290/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1290/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1290/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/12371201/HADOOP-2344.patch against trunk revision r601961. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests +1. The patch passed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1290/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1290/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1290/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1290/console This message is automatically generated.
          Hide
          Doug Cutting added a comment -

          This looks mostly good. A few problems:

          • public and protected methods on public classes need javadoc
          • SimpleCommandExecutor needs javadoc
          • inReader and errReader should be closed in a 'finally' clause
          • there's an empty 'else' clause that should be deleted.
          Show
          Doug Cutting added a comment - This looks mostly good. A few problems: public and protected methods on public classes need javadoc SimpleCommandExecutor needs javadoc inReader and errReader should be closed in a 'finally' clause there's an empty 'else' clause that should be deleted.
          Hide
          Amar Kamat added a comment -

          Process.destroy() in java.lang.UNIXProcess indicates that a call to destroy() clears the streams. So I thought its redundant to invoke it explicitly, no?

          Show
          Amar Kamat added a comment - Process.destroy() in java.lang.UNIXProcess indicates that a call to destroy() clears the streams. So I thought its redundant to invoke it explicitly, no?
          Hide
          Arun C Murthy added a comment -

          Regarding the "Process.waitFor() hangs" issue, this patch clears the error stream using a separate thread while the input stream is read in the current thread (so that they happen in parallel). Once the streams are cleared, ShellCommand waits for the process to terminate and come out of waitFor(). Finally the process is destroyed closing the input/error streams. As done in ShellUtil, the process is made accessible outside ShellCommand so that the process can be externally terminated.

          Amar, I'm not very sure this is the right approach.

          We've had this same structure before and had to remove it since we had a number of issues with threads reading the exec'ed process's stdout/stderr streams which kept hanging, and Owen finally had to resort to bash redirections: see HADOOP-1553 for details.

          Show
          Arun C Murthy added a comment - Regarding the "Process.waitFor() hangs" issue, this patch clears the error stream using a separate thread while the input stream is read in the current thread (so that they happen in parallel). Once the streams are cleared, ShellCommand waits for the process to terminate and come out of waitFor(). Finally the process is destroyed closing the input/error streams. As done in ShellUtil, the process is made accessible outside ShellCommand so that the process can be externally terminated. Amar, I'm not very sure this is the right approach. We've had this same structure before and had to remove it since we had a number of issues with threads reading the exec'ed process's stdout/stderr streams which kept hanging, and Owen finally had to resort to bash redirections: see HADOOP-1553 for details.
          Hide
          Amar Kamat added a comment -

          Sorry for the confusion about UNIXProcess. I am using sun java. So as pointed out by Doug I should close the streams before invoking Process.destroy().

          Show
          Amar Kamat added a comment - Sorry for the confusion about UNIXProcess . I am using sun java. So as pointed out by Doug I should close the streams before invoking Process.destroy() .
          Hide
          Doug Cutting added a comment -

          Arun: In HADOOP-1553, I don't recall hung threads as an issue, but rather simply performance. We never figured out why the performance was bad when using Java threads to handle voluminous sub-process output. But, in this case, where we don't expect huge outputs, that may be less of an issue.

          It might be worth developing a standalone benchmark that repeatedly spawns a process that writes to standard error and/or output in order to test various approaches. If we cannot figure out how to write an efficient thread-based version, then perhaps we should change ShellCommand to redirect outputs to temporary files, but I'd rather avoid that if we can.

          Show
          Doug Cutting added a comment - Arun: In HADOOP-1553 , I don't recall hung threads as an issue, but rather simply performance. We never figured out why the performance was bad when using Java threads to handle voluminous sub-process output. But, in this case, where we don't expect huge outputs, that may be less of an issue. It might be worth developing a standalone benchmark that repeatedly spawns a process that writes to standard error and/or output in order to test various approaches. If we cannot figure out how to write an efficient thread-based version, then perhaps we should change ShellCommand to redirect outputs to temporary files, but I'd rather avoid that if we can.
          Hide
          Amar Kamat added a comment -

          Changes made to the patch other than the javadoc changes

          • logging some of the exceptions in ShellCommand and throwing the main/imp ones.
          • closing the streams explicitly.
          • ignoring the error of df -k since exit code provides the required error information

            comments ?

          Show
          Amar Kamat added a comment - Changes made to the patch other than the javadoc changes logging some of the exceptions in ShellCommand and throwing the main/imp ones. closing the streams explicitly. ignoring the error of df -k since exit code provides the required error information 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/12371486/HADOOP-2344.patch
          against trunk revision r603428.

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

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

          javac +1. The applied patch does not generate any new compiler warnings.

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

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

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

          Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1326/testReport/
          Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1326/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1326/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1326/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/12371486/HADOOP-2344.patch against trunk revision r603428. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests +1. The patch passed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1326/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1326/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1326/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1326/console This message is automatically generated.
          Hide
          Doug Cutting added a comment -

          +1 This looks good to me.

          Has anyone tested to see if this fixes HADOOP-2231?

          Show
          Doug Cutting added a comment - +1 This looks good to me. Has anyone tested to see if this fixes HADOOP-2231 ?
          Hide
          Amar Kamat added a comment -

          Christian could you plz try this patch and let us know the outcome?

          Show
          Amar Kamat added a comment - Christian could you plz try this patch and let us know the outcome?
          Hide
          Raghu Angadi added a comment -

          Good fix. There is one more possibility of deadlock (might be theoritical):
          If processResult() throws an IOException, then we don't drain OutputStream and wait for error reader to reach EOF.

          Show
          Raghu Angadi added a comment - Good fix. There is one more possibility of deadlock (might be theoritical): If processResult() throws an IOException, then we don't drain OutputStream and wait for error reader to reach EOF.
          Hide
          Amar Kamat added a comment -

          Two things,

          • whatever happens, ShellCommand.run() makes sure that an attempt is made to close the streams and destroy the the process in the end of each run.
          • one can always redirect the error to /dev/null as the patch does in case of DF so that explicit command-error handling is not required. This is done to avoid the buffer related issues with respect to error streams.
            comments?
          Show
          Amar Kamat added a comment - Two things, whatever happens, ShellCommand.run() makes sure that an attempt is made to close the streams and destroy the the process in the end of each run. one can always redirect the error to /dev/null as the patch does in case of DF so that explicit command-error handling is not required. This is done to avoid the buffer related issues with respect to error streams. comments?
          Hide
          Raghu Angadi added a comment -

          > Two things,
          Are these about my comment above?

          Show
          Raghu Angadi added a comment - > Two things, Are these about my comment above?
          Hide
          Amar Kamat added a comment -

          Yeah. This could possibly lead to a deadlock. I will fix that soon.

          Show
          Amar Kamat added a comment - Yeah. This could possibly lead to a deadlock. I will fix that soon.
          Hide
          Amar Kamat added a comment -

          Uploading the modified patch. Comments ?

          Show
          Amar Kamat added a comment - Uploading the modified patch. 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/12371743/HADOOP-2344.patch
          against trunk revision r604451.

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

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

          javac +1. The applied patch does not generate any new compiler warnings.

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

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

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

          Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1359/testReport/
          Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1359/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1359/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1359/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/12371743/HADOOP-2344.patch against trunk revision r604451. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests +1. The patch passed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1359/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1359/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1359/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1359/console This message is automatically generated.
          Hide
          Doug Cutting added a comment -

          A few questions about naming:

          • should we move this class to the util package, since it's not fs-specific?
          • should SimpleCommandExecutor#getResponse() instead be named getOutput()?
          Show
          Doug Cutting added a comment - A few questions about naming: should we move this class to the util package, since it's not fs-specific? should SimpleCommandExecutor#getResponse() instead be named getOutput()?
          Hide
          Amar Kamat added a comment -

          A few questions about naming:

          • should we move this class to the util package, since it's not fs-specific?

          +1, since we are using it like a shell.

          • should SimpleCommandExecutor#getResponse() instead be named getOutput()?

          Did you mean SimpleCommandExecutor#getReply()?
          output is more appropriate as in output of a command (execution).

          Show
          Amar Kamat added a comment - A few questions about naming: should we move this class to the util package, since it's not fs-specific? +1, since we are using it like a shell . should SimpleCommandExecutor#getResponse() instead be named getOutput()? Did you mean SimpleCommandExecutor#getReply() ? output is more appropriate as in output of a command (execution) .
          Hide
          Doug Cutting added a comment -

          Did you mean SimpleCommandExecutor#getReply()?

          Yes.

          Show
          Doug Cutting added a comment - Did you mean SimpleCommandExecutor#getReply()? Yes.
          Hide
          Arun C Murthy added a comment -

          Cancelling this patch while Doug's comments get incorporated...

          Show
          Arun C Murthy added a comment - Cancelling this patch while Doug's comments get incorporated...
          Hide
          Amar Kamat added a comment -

          Incorporated Doug's comment.

          Show
          Amar Kamat added a comment - Incorporated Doug's comment.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12372087/HADOOP-2344.patch
          against trunk revision r606058.

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

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

          javac +1. The applied patch does not generate any new compiler warnings.

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

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

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

          Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1419/testReport/
          Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1419/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1419/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1419/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/12372087/HADOOP-2344.patch against trunk revision r606058. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests +1. The patch passed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1419/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1419/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1419/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1419/console This message is automatically generated.
          Hide
          Devaraj Das added a comment -

          Some comments:
          1) The implementation of the deprecated classes should be using the new code.
          2) The javadoc needs @link for the deprecated classes.
          3) In TaskRunner.kill, a check for shexec being null should be made
          4) Should SimpleCommandExecutor be non-static?

          Show
          Devaraj Das added a comment - Some comments: 1) The implementation of the deprecated classes should be using the new code. 2) The javadoc needs @link for the deprecated classes. 3) In TaskRunner.kill, a check for shexec being null should be made 4) Should SimpleCommandExecutor be non-static?
          Hide
          Amar Kamat added a comment -

          Incorporating Devaraj's comments. Submitting a new patch applicable to trunk.

          Show
          Amar Kamat added a comment - Incorporating Devaraj's comments. Submitting a new patch applicable to trunk.
          Hide
          Devaraj Das added a comment -

          I propose that we mark this issue for 0.16. One reason is that this patch deprecates classes that were there in the previous releases of 0.15. Also, this patch requires work to make it apply to 0.15 branch. Thoughts?

          Show
          Devaraj Das added a comment - I propose that we mark this issue for 0.16. One reason is that this patch deprecates classes that were there in the previous releases of 0.15. Also, this patch requires work to make it apply to 0.15 branch. Thoughts?
          Hide
          Christian Kunz added a comment -

          Changed fix version to 0.16.0 on request by Devaraj.

          Show
          Christian Kunz added a comment - Changed fix version to 0.16.0 on request by Devaraj.
          Hide
          Arun C Murthy added a comment -

          There are some minor nits (broken links etc.), I'll supply a new patch...

          Show
          Arun C Murthy added a comment - There are some minor nits (broken links etc.), I'll supply a new patch...
          Hide
          Arun C Murthy added a comment -

          Updated patch for Amar...

          1. Fixed some broken links.
          2. Fixed the logger in Shell.java
          3. Renamed SimpleCommandExecutor to ShellCommandExecutor.
          4. Added some documentation.

          Show
          Arun C Murthy added a comment - Updated patch for Amar... 1. Fixed some broken links. 2. Fixed the logger in Shell.java 3. Renamed SimpleCommandExecutor to ShellCommandExecutor . 4. Added some documentation.
          Hide
          Arun C Murthy added a comment -

          Submitting patch, changed fix version to 0.16.0.

          Show
          Arun C Murthy added a comment - Submitting patch, changed fix version to 0.16.0.
          Hide
          Amar Kamat added a comment -

          +1. Seems fine.

          Show
          Amar Kamat added a comment - +1. Seems fine.
          Hide
          Arun C Murthy added a comment -

          Resubmitting to Mr. Hudson, who is back from vacation...

          Show
          Arun C Murthy added a comment - Resubmitting to Mr. Hudson, who is back from vacation...
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12372246/HADOOP-2344.patch
          against trunk revision r607330.

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

          javadoc -1. The javadoc tool appears to have generated messages.

          javac +1. The applied patch does not generate any new compiler warnings.

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

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

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

          Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1436/testReport/
          Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1436/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1436/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1436/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/12372246/HADOOP-2344.patch against trunk revision r607330. @author +1. The patch does not contain any @author tags. javadoc -1. The javadoc tool appears to have generated messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests -1. The patch failed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1436/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1436/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1436/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1436/console This message is automatically generated.
          Hide
          Arun C Murthy added a comment -

          I just committed this. Thanks, Amar!

          Show
          Arun C Murthy added a comment - I just committed this. Thanks, Amar!
          Hide
          Hudson added a comment -
          Show
          Hudson added a comment - Integrated in Hadoop-Nightly #353 (See http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Nightly/353/ )
          Hide
          Raghu Angadi added a comment -

          This patch has a Javadoc warning.

          Show
          Raghu Angadi added a comment - This patch has a Javadoc warning.
          Hide
          Arun C Murthy added a comment -

          Ugh, the long story is that a similar patch didn't generate a javadoc... sad excuse. My bad.

          I filed/fixed HADOOP-2511 to fix the javadoc warning.

          Show
          Arun C Murthy added a comment - Ugh, the long story is that a similar patch didn't generate a javadoc... sad excuse. My bad. I filed/fixed HADOOP-2511 to fix the javadoc warning.
          Hide
          Raghu Angadi added a comment -

          While working on HADOOP-2420, I noticed that error buffer (message in IOException) was inconsistent. I guess that is because 'completed' was not set to true immediately after waitFor() returns. Should I fix it as part of HADOOP-2420? Its a small change.

          Show
          Raghu Angadi added a comment - While working on HADOOP-2420 , I noticed that error buffer (message in IOException) was inconsistent. I guess that is because 'completed' was not set to true immediately after waitFor() returns. Should I fix it as part of HADOOP-2420 ? Its a small change.
          Hide
          Amar Kamat added a comment -

          The way the Exception message is composed should be changed. Basically its composed of

          • Error stream output
          • Exit code error message

          So

           exitCode = process.waitFor();
           if (exitCode != 0) {
             if (errMsg.length() == 0) {
               errMsg.append("Command exit with status code " + exitCode);
             }
             throw new IOException(errMsg.toString());
           }
          completed = true;
          

          should be changed to

           exitCode = process.waitFor();
           if (exitCode != 0) {
             if (errMsg.length() == 0) {
               errMsg.insert(0,"Command exit with status code " + exitCode + ",\n");
             }
             throw new IOException(errMsg.toString());
           }
           completed = true;
          
          Show
          Amar Kamat added a comment - The way the Exception message is composed should be changed. Basically its composed of Error stream output Exit code error message So exitCode = process.waitFor(); if (exitCode != 0) { if (errMsg.length() == 0) { errMsg.append( "Command exit with status code " + exitCode); } throw new IOException(errMsg.toString()); } completed = true ; should be changed to exitCode = process.waitFor(); if (exitCode != 0) { if (errMsg.length() == 0) { errMsg.insert(0, "Command exit with status code " + exitCode + ",\n" ); } throw new IOException(errMsg.toString()); } completed = true ;
          Hide
          Raghu Angadi added a comment -
          Show
          Raghu Angadi added a comment - See HADOOP-2512 .

            People

            • Assignee:
              Amar Kamat
              Reporter:
              Amar Kamat
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development