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

Szilard Nemeth edited comment on YARN-9886 at 11/9/19 1:29 PM:
---------------------------------------------------------------

Hi [~kmarton] !

Thanks for this patch!

About your question about the documentation: I'm not aware of any generic place 
for scheduler-level info in the documentation.

First of all, can you check the checkstyle issues?

Some comments:

1. In {{yarn-default.xml}}, I would rephrase the description for 
{{"application-tag.based-placement.enable"}} a bit:
{code:java}
Whether to enable application placement based on user ID passed via
 application tags. When it is enabled, u=<userId> pattern will be checked
 and if found, the application will be placed onto the found user's queue,
 if the original user has enough rights on the passed user's queue.{code}
2. You introduced 2 new types of configs: 
{{application-tag.based-placement.enable}} and 
{{application-tag.based-placement.username.whitelist}}.
 Both of them are starting with {{"application-tag.based-placement"}}.
 For me, a more readable / intuitive name would be: 
{{application-tag-based-placement}}, so I don't think you need the dot in this 
name. The rest of the property names look good to me.

3. There's a log string in {{RMAppManager#getUserNameForPlacement}}:
{code:java}
LOG.warn("[{}] user is not allowed to do placement based " +
 "on application tag");{code}
You don't pass the username as an argument to warn, so this is missing.
 On top of that, I'd repharse the beginning of the log message as: "User '{}' 
is not allowed...".
 I'd also recheck all the log strings and would use the "User '<username goes 
here>'" format instead of "[{}] user".

4. In {{RMAppManager#getUserNameForPlacement}}:
{code:java}
LOG.warn("There is no userId passed. The placement is done fot [{}] user",
 user);{code}
There's a typo in this message: "fot" -> "for".
 Could you also modify the message, like:
{code:java}
"userId was not found in application tags."{code}
or something like this?

4. In {{RMAppManager#getUserNameFromApplicationTag}}: 
 You don't need a variable for the {{userIdTag}}. You can simply return the 
value from the loop instead of using the break statement.

5. In {{TestRmAppManager#setApplicationTags}}: 
 The for-loop could be replaced with:
{code:java}
Collections.addAll(applicationTags, tags);{code}
6. Can you rename {{TestAppManager#checkUsername}} to 
{{verifyPlacementUsername}}?

7. Nit: In {{TestAppManager}}, some test methods are starting with an 
unnecessary empty line.

Nice job with the testcases, I really liked them.

Thanks!


was (Author: snemeth):
Hi @kmarton!

Thanks for this patch!

About your question about the documentation: I'm not aware of any generic place 
for scheduler-level info in the documentation.

First of all, can you check the checkstyle issues?

Some comments:

1. In {{yarn-default.xml}}, I would rephrase the description for 
{{"application-tag.based-placement.enable"}} a bit: 
{code:java}
Whether to enable application placement based on user ID passed via
 application tags. When it is enabled, u=<userId> pattern will be checked
 and if found, the application will be placed onto the found user's queue,
 if the original user has enough rights on the passed user's queue.{code}
2. You introduced 2 new types of configs: 
{{application-tag.based-placement.enable}} and 
{{application-tag.based-placement.username.whitelist}}.
 Both of them are starting with {{"application-tag.based-placement"}}.
 For me, a more readable / intuitive name would be: 
{{application-tag-based-placement}}, so I don't think you need the dot in this 
name. The rest of the property names look good to me.

3. There's a log string in {{RMAppManager#getUserNameForPlacement}}: 
{code:java}
LOG.warn("[{}] user is not allowed to do placement based " +
 "on application tag");{code}

 You don't pass the username as an argument to warn, so this is missing.
 On top of that, I'd repharse the beginning of the log message as: "User '{}' 
is not allowed...".
 I'd also recheck all the log strings and would use the "User '<username goes 
here>'" format instead of "[{}] user".

4. In {{RMAppManager#getUserNameForPlacement}}:
{code:java}
LOG.warn("There is no userId passed. The placement is done fot [{}] user",
 user);{code}
There's a typo in this message: "fot" -> "for".
 Could you also modify the message, like:
{code:java}
"userId was not found in application tags."{code}
or something like this?

4. In {{RMAppManager#getUserNameFromApplicationTag}}: 
 You don't need a variable for the {{userIdTag}}. You can simply return the 
value from the loop instead of using the break statement.

5. In {{TestRmAppManager#setApplicationTags}}: 
 The for-loop could be replaced with:
{code:java}
Collections.addAll(applicationTags, tags);{code}
6. Can you rename {{TestAppManager#checkUsername}} to 
{{verifyPlacementUsername}}?

7. Nit: In {{TestAppManager}}, some test methods are starting with an 
unnecessary empty line.


 Nice job with the testcases, I really liked them.

Thanks!

> Queue mapping based on userid passed through application tag
> ------------------------------------------------------------
>
>                 Key: YARN-9886
>                 URL: https://issues.apache.org/jira/browse/YARN-9886
>             Project: Hadoop YARN
>          Issue Type: Improvement
>          Components: scheduler
>            Reporter: Kinga Marton
>            Assignee: Kinga Marton
>            Priority: Major
>         Attachments: YARN-9886-WIP.patch, YARN-9886.001.patch, 
> YARN-9886.002.patch, YARN-9886.003.patch
>
>
> There are situations when the real submitting user differs from the user what 
> arrives to YARN. For example in case of a Hive application when Hive 
> impersonation is turned off, the hive queries will run as Hive user and the 
> mapping is done based on this username. Unfortunately in this case YARN 
> doesn't have any information about the real user and there are cases when the 
> customer may want to map these applications to the real submitting user's 
> queue instead of the Hive queue.
> For these cases, if they would pass the username in the application tag we 
> may read it and use it during the queue mapping, if that user has rights to 
> run on the real user's queue.  
> [~sunilg] please correct me if I missed something.
>  



--
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