Hadoop Common
  1. Hadoop Common
  2. HADOOP-6453

Hadoop wrapper script shouldn't ignore an existing JAVA_LIBRARY_PATH

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.20.2, 0.21.0, 0.22.0
    • Fix Version/s: 0.22.1
    • Component/s: scripts
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Target Version/s:

      Description

      Currently the hadoop wrapper script assumes its the only place that uses JAVA_LIBRARY_PATH and initializes it to a blank line.

      JAVA_LIBRARY_PATH=''

      This prevents anyone from setting this outside of the hadoop wrapper (say hadoop-config.sh) for their own native libraries.

      The fix is pretty simple. Don't initialize it to '' and append the native libs like normal.

      1. HADOOP-6453-trunkv3.patch
        1 kB
        Chad Metcalf
      2. HADOOP-6453-trunkv2.patch
        1 kB
        Chad Metcalf
      3. HADOOP-6453-0.20v3.patch
        1 kB
        Matt Foley
      4. HADOOP-6453-0.20v2.patch
        1 kB
        Chad Metcalf
      5. HADOOP-6453-0.20.patch
        0.9 kB
        Chad Metcalf
      6. HADOOP-6453.trunk.patch
        1.0 kB
        Chad Metcalf

        Issue Links

          Activity

          Hide
          Chad Metcalf added a comment -

          Low risk patch. Doesn't actually change the default behavior of the wrapper script. It just allows users to pass in additional libs in the path.

          Show
          Chad Metcalf added a comment - Low risk patch. Doesn't actually change the default behavior of the wrapper script. It just allows users to pass in additional libs in the path.
          Hide
          Hadoop QA added a comment -

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

          +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 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/217/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/12428348/HADOOP-6453v1.patch against trunk revision 891511. +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 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/217/console This message is automatically generated.
          Hide
          Chad Metcalf added a comment -

          Clarification this is a 0.20.x only patch. I didn't find any JAVA_LIBRARY_PATH in trunk so I'm guessing its been obsoleted?

          Show
          Chad Metcalf added a comment - Clarification this is a 0.20.x only patch. I didn't find any JAVA_LIBRARY_PATH in trunk so I'm guessing its been obsoleted?
          Hide
          Chad Metcalf added a comment -

          0.20.x version

          Show
          Chad Metcalf added a comment - 0.20.x version
          Hide
          Chad Metcalf added a comment -

          Its been a long week. This moved to hadoop-config in a post split world. This problem exists in trunk and 20.x.

          There are no tests, its a wrapper script. Doesn't change the default behavior except for the unlikely case where people were setting JAVA_LIBRARY_PATH and it was getting stomped on and when its no longer stomped on they'll break. Very unlikely case.

          Show
          Chad Metcalf added a comment - Its been a long week. This moved to hadoop-config in a post split world. This problem exists in trunk and 20.x. There are no tests, its a wrapper script. Doesn't change the default behavior except for the unlikely case where people were setting JAVA_LIBRARY_PATH and it was getting stomped on and when its no longer stomped on they'll break. Very unlikely case.
          Hide
          Kevin Weil added a comment -

          We (Twitter) would love to see this patch committed – I should have submitted it 6 months ago. Our deploy process currently involves updating to a new version and then manually commenting out the line that empties the JAVA_LIBRARY_PATH environment variable (i.e. an inferior version of applying Chad's patch). We want to set JAVA_LIBRARY_PATH in hadoop-env.sh and have it used elsewhere, which this patch makes possible.

          Show
          Kevin Weil added a comment - We (Twitter) would love to see this patch committed – I should have submitted it 6 months ago. Our deploy process currently involves updating to a new version and then manually commenting out the line that empties the JAVA_LIBRARY_PATH environment variable (i.e. an inferior version of applying Chad's patch). We want to set JAVA_LIBRARY_PATH in hadoop-env.sh and have it used elsewhere, which this patch makes possible.
          Hide
          Hadoop QA added a comment -

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

          +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 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/Hadoop-Patch-h4.grid.sp2.yahoo.net/243/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/243/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/243/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/243/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/12428360/HADOOP-6453.trunk.patch against trunk revision 893666. +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 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/Hadoop-Patch-h4.grid.sp2.yahoo.net/243/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/243/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/243/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/243/console This message is automatically generated.
          Hide
          Chad Metcalf added a comment -

          No test included for change to wrapper script.

          Show
          Chad Metcalf added a comment - No test included for change to wrapper script.
          Hide
          steve_l added a comment -

          I don't use the startup scripts, so it doesn't have any effect to me, but I see the benefit for anyone who has native libs they do want in the runtime.

          1. There is a risk that this will break things, namely any system that has a JAVA_LIBRARY_PATH env variable set to something that is incompatible with HADOOP. This is the usual badly-configured CLASSPATH problem, the reason a lot of java launcher scripts either ignore that env variable completely, or provide some way to disable it (e.g. Ant's -noclasspath option)
          1. Even if there is no automated test, someone needs to manually check that if the existing value of JAVA_LIBRARY_PATH is set to something with spaces in, such as c:\Program Files\some application\ then everything still propagates correctly. As usual, Cygwin will be the troublespot here.
          Show
          steve_l added a comment - I don't use the startup scripts, so it doesn't have any effect to me, but I see the benefit for anyone who has native libs they do want in the runtime. There is a risk that this will break things, namely any system that has a JAVA_LIBRARY_PATH env variable set to something that is incompatible with HADOOP. This is the usual badly-configured CLASSPATH problem, the reason a lot of java launcher scripts either ignore that env variable completely, or provide some way to disable it (e.g. Ant's -noclasspath option) Even if there is no automated test, someone needs to manually check that if the existing value of JAVA_LIBRARY_PATH is set to something with spaces in, such as c:\Program Files\some application\ then everything still propagates correctly. As usual, Cygwin will be the troublespot here.
          Hide
          steve_l added a comment -


          I should add that it's script patches that have the greatest cost/line in Ant; we dropped support for win9x not because java didn't work on it, but the .BAT syntax was weaker and we couldn't face debugging it. Windows + Cygwin is equally trouble, so before any patch goes in we really need to think about how to test on that platform.

          Show
          steve_l added a comment - I should add that it's script patches that have the greatest cost/line in Ant; we dropped support for win9x not because java didn't work on it, but the .BAT syntax was weaker and we couldn't face debugging it. Windows + Cygwin is equally trouble, so before any patch goes in we really need to think about how to test on that platform.
          Hide
          Chad Metcalf added a comment -

          Added:

          JAVA_LIBRARY_PATH=`cygpath -p -w "$JAVA_LIBRARY_PATH"`
          

          For trunk and 0.20 in the appropriate places. This now receives the same level of protection that CLASSPATH does. As I acknowledged above there is indeed a possibility that a user will have a broken JAVA_LIBRARY_PATH and this will break for them. That possibility is slim. A quick google of JAVA_LIBRARY_PATH brings this jira up as #5 which indicates its a significantly less common env then CLASSPATH. Regardless the possibility exists.

          With the cygpath fix above this change makes every reasonable effort to protect cygwin users. Which places them in the same boat as all the other bash users (Linux, Mac, Solaris, etc). If you have a broken env there is nothing I can do. It would the same if you had a broken CLASSPATH. As with all possibly breaking changes there is a trade off between the users that need the functionality and the users it will break.

          Users that need to get native libraries in as it stands have to hack the wrapper. This is tedious and error prone over setting a env. And its not immediately obvious that its what you have to do. The wrapper script respects CLASSPATH logically it should respect others. And its not documented that if you want to do this, oh by the way you have to hack this.

          I'm all for testing. But the risk here is broken user env's which is not something in the scope of this jira to address. We can test the patch and add a release note so that if it breaks someone they can read that this was changed. If people are really that worried about it we can add:

          echo "Existing JAVA_LIBRARY_PATH detected, if things are breaking please check this first."
          

          But most likely that will just send someone who breaks for a different reason off on a 5 minute wild goose chase.

          Show
          Chad Metcalf added a comment - Added: JAVA_LIBRARY_PATH=`cygpath -p -w "$JAVA_LIBRARY_PATH" ` For trunk and 0.20 in the appropriate places. This now receives the same level of protection that CLASSPATH does. As I acknowledged above there is indeed a possibility that a user will have a broken JAVA_LIBRARY_PATH and this will break for them. That possibility is slim. A quick google of JAVA_LIBRARY_PATH brings this jira up as #5 which indicates its a significantly less common env then CLASSPATH . Regardless the possibility exists. With the cygpath fix above this change makes every reasonable effort to protect cygwin users. Which places them in the same boat as all the other bash users (Linux, Mac, Solaris, etc). If you have a broken env there is nothing I can do. It would the same if you had a broken CLASSPATH . As with all possibly breaking changes there is a trade off between the users that need the functionality and the users it will break. Users that need to get native libraries in as it stands have to hack the wrapper. This is tedious and error prone over setting a env. And its not immediately obvious that its what you have to do. The wrapper script respects CLASSPATH logically it should respect others. And its not documented that if you want to do this, oh by the way you have to hack this. I'm all for testing. But the risk here is broken user env's which is not something in the scope of this jira to address. We can test the patch and add a release note so that if it breaks someone they can read that this was changed. If people are really that worried about it we can add: echo "Existing JAVA_LIBRARY_PATH detected, if things are breaking please check this first." But most likely that will just send someone who breaks for a different reason off on a 5 minute wild goose chase.
          Hide
          steve_l added a comment -

          I agree, handling broken env variables is a losing battle. They can break it in so many ways that it's impossible to fix; the classic enemy is the quotation marks halfway through PATH; this screws up the quote logic and your scripts don't stand a chance, not in anything vaguely cross platform. Perl or Python entry scripts, that's a different story.

          For bash, the cygpath patch should be enough.

          I've been thinking about how to test this. TestCLI can be used to run stuff on the command line, all we need is a hadoop diagnostics command whose output includes all the env variables. If JAVA_LIBRARY_PATH is then set to values with and without a space in in different test cases (that will be the tricky bit; may need some extensions to the cli execution), the test could look for the value in the output. I will file that as a separate feature.

          Show
          steve_l added a comment - I agree, handling broken env variables is a losing battle. They can break it in so many ways that it's impossible to fix; the classic enemy is the quotation marks halfway through PATH; this screws up the quote logic and your scripts don't stand a chance, not in anything vaguely cross platform. Perl or Python entry scripts, that's a different story. For bash, the cygpath patch should be enough. I've been thinking about how to test this. TestCLI can be used to run stuff on the command line, all we need is a hadoop diagnostics command whose output includes all the env variables. If JAVA_LIBRARY_PATH is then set to values with and without a space in in different test cases (that will be the tricky bit; may need some extensions to the cli execution), the test could look for the value in the output. I will file that as a separate feature.
          Hide
          Chad Metcalf added a comment -

          Confused as to the remaining open actions on this JIRA. What more needs to be done?

          Show
          Chad Metcalf added a comment - Confused as to the remaining open actions on this JIRA. What more needs to be done?
          Hide
          steve_l added a comment -

          I think its OK as is, we just need to (currently manually) check that things work on cygwin. To automate a command line check needs the cli tester to excec the shell scripts, which it doesnt (yet), or use some other testrunner antunit is the obvious choice

          Show
          steve_l added a comment - I think its OK as is, we just need to (currently manually) check that things work on cygwin. To automate a command line check needs the cli tester to excec the shell scripts, which it doesnt (yet), or use some other testrunner antunit is the obvious choice
          Hide
          Kevin Weil added a comment -

          So are we good to go on this, or are there any more modifications desired? We're happy with the current patch FWIW.

          Show
          Kevin Weil added a comment - So are we good to go on this, or are there any more modifications desired? We're happy with the current patch FWIW.
          Hide
          Jakob Homan added a comment -

          The trunk patch has unfortunately gone stale. However, this seems like a good change to make with essentially no risk. This will treat JAVA_LIBRARY_PATH consistently with JAVA_HOME, and can be marked as incompatible change. cygwin should be fine. Chad, can you please spin up a new patch and, barring any objections, I'll review and commit it.

          Show
          Jakob Homan added a comment - The trunk patch has unfortunately gone stale. However, this seems like a good change to make with essentially no risk. This will treat JAVA_LIBRARY_PATH consistently with JAVA_HOME, and can be marked as incompatible change. cygwin should be fine. Chad, can you please spin up a new patch and, barring any objections, I'll review and commit it.
          Hide
          Chad Metcalf added a comment -

          Thanks.

          Show
          Chad Metcalf added a comment - Thanks.
          Hide
          Chad Metcalf added a comment -

          Thanks

          Show
          Chad Metcalf added a comment - Thanks
          Hide
          Jakob Homan added a comment -

          +1

          Show
          Jakob Homan added a comment - +1
          Hide
          Jakob Homan added a comment -

          Hudson has completely disappeared and test-patch was already run on a prior version of this patch, so I'm going to go ahead and commit this as-is.

          Show
          Jakob Homan added a comment - Hudson has completely disappeared and test-patch was already run on a prior version of this patch, so I'm going to go ahead and commit this as-is.
          Hide
          Jakob Homan added a comment -

          I've committed this. Resolving as fixed. Thanks, Chad!

          Show
          Jakob Homan added a comment - I've committed this. Resolving as fixed. Thanks, Chad!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #432 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/432/)
          HADOOP-6453. Hadoop wrapper script shouldn't ignore an existing JAVA_LIBRARY_PATH. Contributed by Chad Metcalf.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #432 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/432/ ) HADOOP-6453 . Hadoop wrapper script shouldn't ignore an existing JAVA_LIBRARY_PATH. Contributed by Chad Metcalf.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #364 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/364/)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #364 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/364/ )
          Hide
          Matt Foley added a comment -

          Reopened for port to 0.20-security, per Tim Broberg's request.

          Show
          Matt Foley added a comment - Reopened for port to 0.20-security, per Tim Broberg's request.
          Hide
          Matt Foley added a comment -

          Re-based to branch-0.20-security-205. Won't mark "Patch Available" as isn't for trunk.

          Please review.

          Show
          Matt Foley added a comment - Re-based to branch-0.20-security-205. Won't mark "Patch Available" as isn't for trunk. Please review.
          Hide
          Tim Broberg added a comment -

          if [ -e "$

          {HADOOP_PREFIX}/lib/libhadoop.a" ]; then
          JAVA_LIBRARY_PATH=${HADOOP_PREFIX}

          /lib
          fi

          It appears that the patched script discards all other JAVA_LIBRARY_PATH definitions if there is a libhadoop.a file.

          Is this by design?

          Show
          Tim Broberg added a comment - if [ -e "$ {HADOOP_PREFIX}/lib/libhadoop.a" ]; then JAVA_LIBRARY_PATH=${HADOOP_PREFIX} /lib fi It appears that the patched script discards all other JAVA_LIBRARY_PATH definitions if there is a libhadoop.a file. Is this by design?
          Hide
          Tim Broberg added a comment -

          The same line is causing hadoop-7825 as looks suspicious in the patch for branch-0.20-security-205.

          Show
          Tim Broberg added a comment - The same line is causing hadoop-7825 as looks suspicious in the patch for branch-0.20-security-205.
          Hide
          Matt Foley added a comment -

          Got insufficient feedback on this and HADOOP-7825 to make it into 1.0.0.
          Setting target version to 1.1.0.

          Show
          Matt Foley added a comment - Got insufficient feedback on this and HADOOP-7825 to make it into 1.0.0. Setting target version to 1.1.0.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12503143/HADOOP-6453-0.20v3.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/766//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/12503143/HADOOP-6453-0.20v3.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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/766//console This message is automatically generated.
          Hide
          Matt Foley added a comment -

          Withdrawing patch.

          Show
          Matt Foley added a comment - Withdrawing patch.
          Hide
          Matt Foley added a comment -

          closing for apparent lack of interest.

          Show
          Matt Foley added a comment - closing for apparent lack of interest.

            People

            • Assignee:
              Unassigned
              Reporter:
              Chad Metcalf
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development