[
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: [email protected]
For additional commands, e-mail: [email protected]
