|
[
Permlink
| « Hide
]
Amareshwari Sriramadasu added a comment - 15/May/08 12:01 PM
Preliminary patch for adding LD_LIBRARY_PATH. Meanwhile I will do the testing.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12382104/patch-2867.txt  against trunk revision 656585. +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. +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/2479/testReport/ This message is automatically generated. Amareshwari, please do not blindly add it to the environment - you need to append cwd to the existing LD_LIBRARY_PATH, especially in-light of
Also, please ensure you have a test-case for this. Thanks!
Amareshwari, while you working on this I think it would be great to fix
The -program option in pipes command line allows to add files beyond the executable (comma separated) and they will all end up in the local cache. Some of them might be shared libraries. So far, one has to create a wrapper to configure the appropriate LD_LIBRARY_PATH based on mapred.cache.localFiles.
Therefore, it would be nice if the task could be configured to add all parent directories of mapred.cache.localFiles to LD_LIBRARY_PATH.
The -program option in pipes is expected to take only executable URI. Comma separated files ending up in local cache looks like a hack. Should this be supported? Either we support or not, I think this needs to be documented.
Christian, do you mean the immediate parents of all mapred.cache.localFiles or the whole parent hierarchy of mapred.cache.localFiles?
We need the support for sure, whether implemented less hacky or not.
files in mapred.cache.localFiles usually have their own specific subdirectory, i.e. when one of them is a shared library, then it should be sufficient to include its parent directory into LD_LIBRARY_PATH Here is a patch adding task's cwd and parents of mapred.cache.localFiles to the LD_LIBRARY_PATH. The patch is tested with simple c program which loads a shared library.
@@ -170,6 +175,8 @@
There is no need to add the native libs that have been softlinked to the LD_LIBRARY_PATH, they are already being added to the 'java.library.path' property. @@ -387,9 +394,15 @@ You should not start with an empty Map for the environment, that will trash the existing environment. You should get from ProcessBuilder the Map with the current environment and modify that one. You should get the current value of LD_LIBRARY_PATH (actually to do it properly you should check if you are in windows or unix and get the proper env variable) and append to it the current working dir. For the separator make sure you use 'System.getProperty("path.separator")' instead ':'
In org.apache.hadoop.util.Shell.java, the environment passed is set using builder.environment().putAll(this.environment);
This (http://java.sun.com/j2se/1.5.0/docs/api/java/lang/ProcessBuilder.html#environment( Here is patch after incorporating review comments.
On second thought, adding the directories that may have stuff that is not native libs at all, kind of does not make senese to add them blindly. Since the users can provide symlinks in the cwd for the cached libraries. This is not necessary. I removed the code which adds parent directories of all mapred.cache.localFiles
LD_LIBRARY_PATH is honored by Cygwin, though not by windows. And we support only Cygwin. So, we dont need a check for windows explicitly for LD_LIBRARY_PATH environment variable.
+1 Christian, does this work for you? I only asked for pre-configuring LD_LIBRARY_PATH by the framework for the convenience of the application programmer not to have to write a wrapper when the executable needs some shared libraries not already deployed on the nodes.
No big deal. Arun just told me about mapred.create.symlink I was not aware about.
All my comments in this issue can be completely disregarded. -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12382373/patch-2867.txt against trunk revision 658704. +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. +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/2507/testReport/ This message is automatically generated. I just committed this. Thanks, Amareshwari!
Integrated in Hadoop-trunk #499 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/499/
) Testing this feature requires a shared library, which has to be copied to DistributedCache. Creating a sharedlib is platform dependent and hence I havent put a testcase for this. This change affects pipes and streaming also, since the environment of pipes and streaming tasks comes from the parent java tasks.
I tested this feature by running a streaming application which loads shared library. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
Add a task's cwd to it's LD_LIBRARY_PATH
