Hadoop Common
  1. Hadoop Common
  2. HADOOP-9818

Remove usage of "bash -c" from oah.fs.DF

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 3.0.0, 2.1.0-beta
    • Fix Version/s: None
    • Component/s: None
    • Target Version/s:

      Description

      DF uses "bash -c" to shell out to the unix df command. This is potentially unsafe; let's think about removing it.

      1. HADOOP-9818.patch
        0.7 kB
        Kousuke Saruta

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          2h 13m 1 Andrew Wang 02/Aug/13 00:22
          Allen Wittenauer made changes -
          Labels newbie BB2015-05-TBR newbie
          Hide
          Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 36s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 javac 7m 30s There were no new javac warning messages.
          +1 javadoc 9m 34s There were no new javadoc warning messages.
          +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
          -1 checkstyle 1m 4s The applied patch generated 2 new checkstyle issues (total was 5, now 5).
          +1 whitespace 0m 0s The patch has no lines that end in whitespace.
          +1 install 1m 32s mvn install still works.
          +1 eclipse:eclipse 0m 34s The patch built with eclipse:eclipse.
          +1 findbugs 1m 40s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 common tests 23m 1s Tests passed in hadoop-common.
              59m 59s  



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12595486/HADOOP-9818.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 57d9a97
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/6397/artifact/patchprocess/diffcheckstylehadoop-common.txt
          hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6397/artifact/patchprocess/testrun_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6397/testReport/
          Java 1.7.0_55
          uname Linux asf909.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6397/console

          This message was automatically generated.

          Show
          Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 36s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac 7m 30s There were no new javac warning messages. +1 javadoc 9m 34s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 1m 4s The applied patch generated 2 new checkstyle issues (total was 5, now 5). +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 install 1m 32s mvn install still works. +1 eclipse:eclipse 0m 34s The patch built with eclipse:eclipse. +1 findbugs 1m 40s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 common tests 23m 1s Tests passed in hadoop-common.     59m 59s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12595486/HADOOP-9818.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 57d9a97 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/6397/artifact/patchprocess/diffcheckstylehadoop-common.txt hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6397/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6397/testReport/ Java 1.7.0_55 uname Linux asf909.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6397/console This message was automatically generated.
          Hide
          Kousuke Saruta added a comment -

          I'm writing the test for DF and I've noticed things which may be defects.

          1. DF#getFilesystem() behaves differently depending on calling the method before / after calling DF#getMount().
          I think that is because getMount() calls DF#run() and DF#parseOutput() but getFilesystem() don't.

          2. DF#getMount() calls DF#run(). Thus, df command will be executed every we call DF#getMount().
          I think it's inefficient. Why only getMount() calls run() in itself?
          I think run() and parseOutput() should be called in constructor of DF so that df command will be executed when DF is instantiated.

          Show
          Kousuke Saruta added a comment - I'm writing the test for DF and I've noticed things which may be defects. 1. DF#getFilesystem() behaves differently depending on calling the method before / after calling DF#getMount(). I think that is because getMount() calls DF#run() and DF#parseOutput() but getFilesystem() don't. 2. DF#getMount() calls DF#run(). Thus, df command will be executed every we call DF#getMount(). I think it's inefficient. Why only getMount() calls run() in itself? I think run() and parseOutput() should be called in constructor of DF so that df command will be executed when DF is instantiated.
          Hide
          Colin Patrick McCabe added a comment -

          The "bash -c" and attendant "2>/dev/null" were added by HADOOP-2344. I think the rationale was that having a lot of stderr output would potentially slow down execution (see https://issues.apache.org/jira/browse/HADOOP-1553?focusedCommentId=12514794&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-12514794 )

          I think what we should do in this case is add an option to ShellCommandExceutor that redirects stderr to an instance of com.google.common.io.input.NullInputstream (or similar).

          Show
          Colin Patrick McCabe added a comment - The " bash -c " and attendant " 2>/dev/null " were added by HADOOP-2344 . I think the rationale was that having a lot of stderr output would potentially slow down execution (see https://issues.apache.org/jira/browse/HADOOP-1553?focusedCommentId=12514794&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-12514794 ) I think what we should do in this case is add an option to ShellCommandExceutor that redirects stderr to an instance of com.google.common.io.input.NullInputstream (or similar).
          Colin Patrick McCabe made changes -
          Link This issue duplicates HADOOP-2344 [ HADOOP-2344 ]
          Colin Patrick McCabe made changes -
          Link This issue is duplicated by HDFS-5062 [ HDFS-5062 ]
          Hide
          Kousuke Saruta added a comment -

          Andrew, Thanks!
          I'll try it.

          Show
          Kousuke Saruta added a comment - Andrew, Thanks! I'll try it.
          Hide
          Hadoop QA added a comment -

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

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

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

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

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

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

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2907//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2907//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/12595486/HADOOP-9818.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2907//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2907//console This message is automatically generated.
          Andrew Wang made changes -
          Assignee Kousuke Saruta [ sarutak ]
          Hide
          Andrew Wang added a comment -

          Hi Kousuke, thanks for the patch!

          I've made you a contributor, so now you should be able to assign Common JIRAs to yourself and mark them as Patch Available via the "Submit Patch" button.

          Part of the trickiness from removing "2>&1" is error handling. I haven't looked into DF at all, but ProcessBuilder separates stdout and stderr, so stderr messages no longer show up. So, I added another param to Shell in HADOOP-9652 (hopefully committed soon) which merges them back together.

          It'd be great if you can review the DF error handling behavior, especially adding tests for any missing holes you find.

          Show
          Andrew Wang added a comment - Hi Kousuke, thanks for the patch! I've made you a contributor, so now you should be able to assign Common JIRAs to yourself and mark them as Patch Available via the "Submit Patch" button. Part of the trickiness from removing "2>&1" is error handling. I haven't looked into DF at all, but ProcessBuilder separates stdout and stderr, so stderr messages no longer show up. So, I added another param to Shell in HADOOP-9652 (hopefully committed soon) which merges them back together. It'd be great if you can review the DF error handling behavior, especially adding tests for any missing holes you find.
          Hide
          Kousuke Saruta added a comment -

          I've removed "2>/dev/null" from the patch.
          No problem?

          Show
          Kousuke Saruta added a comment - I've removed "2>/dev/null" from the patch. No problem?
          Andrew Wang made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Kousuke Saruta made changes -
          Field Original Value New Value
          Attachment HADOOP-9818.patch [ 12595486 ]
          Hide
          Kousuke Saruta added a comment -

          I've modified the code not to use "bash -c".

          Show
          Kousuke Saruta added a comment - I've modified the code not to use "bash -c".
          Andrew Wang created issue -

            People

            • Assignee:
              Kousuke Saruta
              Reporter:
              Andrew Wang
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development