You are viewing a plain text version of this content. The canonical link for it is here.
Posted to yarn-issues@hadoop.apache.org by "Peter Bacsko (Jira)" <ji...@apache.org> on 2020/10/09 10:45:00 UTC
[jira] [Comment Edited] (YARN-10425) Replace the legacy placement
engine in CS with the new one
[ 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org