> On Sept. 16, 2012, 8:20 a.m., Mike Percy wrote: > > Hey man, thanks for the patch. I believe this is related to the snappy > > issue raised over in HADOOP-8806, so it should be the java.library.path > > added to LD_LIBRARY_PATH, not the classpath. In the script, it's called > > FLUME_JAVA_LIBRARY_PATH. > > > > Aside from that, I have a couple comments: > > 1. The existing LD_LIBRARY_PATH should be discarded. There is a risk of an > > environment difference between system startup scripts and user shell > > invocations if we don't force environment variables that we depend on to be > > defined in the flume-env.sh script. So I think we should get rid of the > > logic involving SYSTEM_LD_LIBRARY_PATH. The problem occurs when an > > administrator sets LD_LIBRARY_PATH in his/her shell (maybe in their > > .bash_profile) and starts Flume, it runs fine for some time, and when the > > system reboots that variable is not set so Flume crashes on startup. > > 2. The body of the set_LD_LIBRARY_PATH() function can be indented, and I > > think it should be, for readability.
Though I don't essentially believe (1) is a really a problem, I do notice now that the script is doing that for other variables. The reason I believe it is not an issue is that if a user is running it - there is no risk of flume starting in a startup script later. Essentially I don't believe that any well managed setup will allow the script to run both as startup script and as a user script(as such I think clearing all these environment variables in the script is really overkill). I believe it will be only one of the two, else it is just bad set up on the whole. But I will update the script to honor the convention. Sorry about the indentation, didn't realize git diff -w blows away all indentation completely. - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7103/#review11595 ----------------------------------------------------------- On Sept. 14, 2012, 7:41 a.m., Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7103/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2012, 7:41 a.m.) > > > Review request for Flume. > > > Description > ------- > > Added a method to add FLUME_CLASSPATH to LD_LIBRARY_PATH. > > > This addresses bug FLUME-1582. > https://issues.apache.org/jira/browse/FLUME-1582 > > > Diffs > ----- > > bin/flume-ng 121adf3 > > Diff: https://reviews.apache.org/r/7103/diff/ > > > Testing > ------- > > Ran an agent and checked the LD_LIBRARY_PATH. > > > Thanks, > > Hari Shreedharan > >
