[ 
https://issues.apache.org/jira/browse/YARN-2369?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14680872#comment-14680872
 ] 

Jason Lowe commented on YARN-2369:
----------------------------------

Sorry for the long delay, and thanks for your patience.  Comments on the latest 
patch:

The newline after the Configuration.getTrimmedStringCollection was accidentally 
removed

There's a lot of unnecessary import changes in MRApps including conversions to 
using wildcard imports which are unrelated to this JIRA.

An entry for mapreduce.application.appendable.env.variables should be in 
mapred-default.xml with the appropriate default value and description for 
documentation purposes

"ClASSPATH" s/b "CLASSPATH" in the unit test comment

The unit test is making an implicit assumption that MRApps.setClasspath is 
setting the environment variable iteratively, which is not a requirement for 
how it performs semantically.  That could change in the future and break this 
test.  A better unit test would be one that explicitly verifies 
MRapps.addtoEnvironment behavior directly by checking for path append behavior 
on variables in the configured append list and overwrite behavior for variables 
not in that list.

I think the "public static final" should go back on the MRJobConfig additions 
to make it consistent with the rest of the file.  Checkstyle will complain, but 
for this instance I think consistency is better here given the benign nature of 
what it is complaining about.

Arrays.asList seems like simpler way to statically initialize the default value 
in MRJobConfig as there's no need for the anonymous class, serial version 
boilerplate, or invocation of add methods in a static block.

The "/**" line at the end of the MRJobConfig patch hunk had its indentation 
accidentally removed.

I'm wondering if we should move this JIRA to the MAPREDUCE project since that's 
where most of the changes are and where most of the user-visible functionality 
appears.  The extreme case is to file three separate for the three affected 
projects, but that seems extreme for this case.  Any opinions on that?

> Environment variable handling assumes values should be appended
> ---------------------------------------------------------------
>
>                 Key: YARN-2369
>                 URL: https://issues.apache.org/jira/browse/YARN-2369
>             Project: Hadoop YARN
>          Issue Type: Bug
>    Affects Versions: 2.2.0
>            Reporter: Jason Lowe
>            Assignee: Dustin Cote
>         Attachments: YARN-2369-1.patch, YARN-2369-2.patch, YARN-2369-3.patch, 
> YARN-2369-4.patch, YARN-2369-5.patch, YARN-2369-6.patch
>
>
> When processing environment variables for a container context the code 
> assumes that the value should be appended to any pre-existing value in the 
> environment.  This may be desired behavior for handling path-like environment 
> variables such as PATH, LD_LIBRARY_PATH, CLASSPATH, etc. but it is a 
> non-intuitive and harmful way to handle any variable that does not have 
> path-like semantics.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to