[
https://issues.apache.org/jira/browse/YARN-10425?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17210822#comment-17210822
]
Peter Bacsko edited comment on YARN-10425 at 10/9/20, 10:44 AM:
----------------------------------------------------------------
Thanks for the patch [~shuzirra]. I mostly have minor comments & questions:
1. Nits:
* instead of directly using the constructor {{new Groups(conf)}}, you might
want to use {{Groups.getUserToGroupsMappingServiceWithLoadedConfiguration()}}
* the comment "Groups won't refresh it's" --> "its"
2. I think the condition below should not be allowed. If, for whatever reason
we couldn't retrieve the groups service provider, that is a serious error and
we shouldn't proceed any further.
{noformat}
if (groups == null) {
LOG.warn(
"Group provider hasn't been set, cannot query groups for user
{}",
user);
return;
}
{noformat}
What is the rationale behind this change?
3. Same here, don't catch this if we know that the error is group-related:
{noformat}
try {
setupGroupsForVariableContext(vctx, user);
} catch (IOException e) {
LOG.warn("Unable to setup groups: {}", e.getMessage());
}
{noformat}
4. CapacityScheduler.java: don't use "*" imports
5. TestUserGroupMappingPlacementRule.java: the test {{testNullGroupMapping()}}
no longer throws exception because of #2.
6. TestQueueMappings.java: don't use "*" imports
was (Author: pbacsko):
Thanks for the patch [~shuzirra]. I mostly have minor comments & questions:
1. Nits:
* instead of directly using the constructor {{new Groups(conf)}}, you might
want to use {{Groups.getUserToGroupsMappingServiceWithLoadedConfiguration()}}
* the comment "Groups won't refresh it's" --> "its"
2. I think the condition below should not be allowed. If, for whatever reason
we couldn't retrieve the groups service provider, that is a serious error and
we shouldn't proceed any further.
{noformat}
if (groups == null) {
LOG.warn(
"Group provider hasn't been set, cannot query groups for user
{}",
user);
return;
}
{noformat}
What is the rationale behind this change?
3. Same here, don't catch this if we know that the error is group-related:
{noformat}
try {
setupGroupsForVariableContext(vctx, user);
} catch (IOException e) {
LOG.warn("Unable to setup groups: {}", e.getMessage());
}
{noformat}
4. CapacityScheduler.java: don't use "*" imports
5. TestUserGroupMappingPlacementRule.java: the test {{testNullGroupMapping()}}
no longer throws exception because of #2.
6. TestQueueMappings.java: don't use "*" imports
> Replace the legacy placement engine in CS with the new one
> ----------------------------------------------------------
>
> Key: YARN-10425
> URL: https://issues.apache.org/jira/browse/YARN-10425
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Gergely Pollak
> Assignee: Gergely Pollak
> Priority: Major
> Attachments: YARN-10425.001.patch
>
>
> Remove the UserGroupMapping and ApplicationName mapping classes, and use the
> new CSMappingPlacementRule instead. Also cleanup the orphan classes which are
> used by these classes only.
--
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
