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

Szilard Nemeth commented on YARN-10779:
---------------------------------------

Hi [~pbacsko],
As per our offline discussion, I think your PoC is fine.
I don't like to say this because it's conceptually not the best to include 
Configuration objects in the PB classes but I can't come up with a better 
approach.
Since hardcoding this behaviour of making all the tags lowercase was also 
conceptually wrong, but users could build on top of this behaviour, we can't 
simply eliminate it. Making it configurable is the way to go.
However, making the casing of tags accross the codebase consistent is the 
easiest to catch on the PB level.
My comments: 
1. Please use YarnConfiguration to store the config name + default value
2. Please add testcases to validate the default behavior + the use-case where 
converting to lowercase is not required.
3. Please also update the documentation related to the app tag letter case, if 
there's any. If not, I'm fine with a follow-up jira as well. As you introduced 
a new config property, the documentation change is a must.

> Add option to disable lowercase conversion in GetApplicationsRequestPBImpl 
> and ApplicationSubmissionContextPBImpl
> -----------------------------------------------------------------------------------------------------------------
>
>                 Key: YARN-10779
>                 URL: https://issues.apache.org/jira/browse/YARN-10779
>             Project: Hadoop YARN
>          Issue Type: Task
>          Components: resourcemanager
>            Reporter: Peter Bacsko
>            Assignee: Peter Bacsko
>            Priority: Major
>         Attachments: YARN-10779-POC.patch
>
>
> In both {{GetApplicationsRequestPBImpl}} and 
> {{ApplicationSubmissionContextPBImpl}}, there is a forced lowercase 
> conversion:
> {noformat}
>     checkTags(tags);
>     // Convert applicationTags to lower case and add
>     this.applicationTags = new TreeSet<>();
>     for (String tag : tags) {
>       this.applicationTags.add(StringUtils.toLowerCase(tag));
>     }
>   }
> {noformat}
> However, we encountered some cases where this is not desirable for "userid" 
> tags. 
> Proposed solution: since both classes are pretty low-level and can be often 
> instantiated, a {{Configuration}} object which loads {{yarn-site.xml}} should 
> be cached inside them. A new property should be created which tells whether 
> lowercase conversion should occur or not.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org

Reply via email to