[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=17270425#comment-17270425
]
Ahmed Hussein edited comment on YARN-10425 at 1/22/21, 8:57 PM:
[~shuzirra], [~pbacsko], [~snemeth], [~BilwaST] thanks for the contribution.
I have a question about the changes introduced by this Ticket. The following
code block
is from
[CSMappingPlacementRule#L128|https://github.com/apache/hadoop/commit/567600fd80896c1c9b0db1f228368d4eb2a694a2#diff-92b5797cf7739d330364d967172e65e61a859c776d9ebe526aba03ea33039033R127]
{code:java}
if (groups == null) {
//We cannot use Groups#getUserToGroupsMappingService here, because when
//tests change the HADOOP_SECURITY_GROUP_MAPPING, Groups won't refresh its
//cached instance of groups, so we might get a Group instance which
//ignores the HADOOP_SECURITY_GROUP_MAPPING settings.
groups = new Groups(conf);
}
{code}
IIUC, the design of groups caching "{{Groups.cache}}" relies on the fact that
the Groups being a singleton. Otherwise, there will be inconsistent behavior
especially in classes like {{JniBasedUnixGroupsNetgroupMapping}} and
{{ShellBasedUnixGroupsNetgroupMapping}}. Both mapping implementations have a
second caching layer for the netgroups "{{NetgroupCache}}".
I have the following two concerns regarding an independent Groups instance in
{{CSMappingPlacementRule.java}}
* It breaks the design leading to inconsistent behaviors that do not match the
expected. As I mentioned, {{NetgroupCache}} contents won't be defined.
* Performance considerations. Allocating "N" instances of {{Groups}} means
fetching the user's groups "N" times. Therefore, Guava cacheLoader's refresh
will be done "N" times, and so on.
Why did you decide to make that change instead of fixing the design of the unit
tests?
IIUC, there is a need to fix that bug in a follow up Jira.
was (Author: ahussein):
[~shuzirra], [~pbacsko] thanks for the contribution.
I have a question about the changes introduced by this Ticket. The following
code block
is from
[CSMappingPlacementRule#L128|https://github.com/apache/hadoop/commit/567600fd80896c1c9b0db1f228368d4eb2a694a2#diff-92b5797cf7739d330364d967172e65e61a859c776d9ebe526aba03ea33039033R127]
{code:java}
if (groups == null) {
//We cannot use Groups#getUserToGroupsMappingService here, because when
//tests change the HADOOP_SECURITY_GROUP_MAPPING, Groups won't refresh its
//cached instance of groups, so we might get a Group instance which
//ignores the HADOOP_SECURITY_GROUP_MAPPING settings.
groups = new Groups(conf);
}
{code}
IIUC, the design of groups caching "{{Groups.cache}}" relies on the fact that
the Groups being a singleton. Otherwise, there will be inconsistent behavior
especially in classes like {{JniBasedUnixGroupsNetgroupMapping}} and
{{ShellBasedUnixGroupsNetgroupMapping}}. Both mapping implementations have a
second caching layer for the netgroups "{{NetgroupCache}}".
I have the following two concerns regarding an independent Groups instance in
{{CSMappingPlacementRule.java}}
* It breaks the design leading to inconsistent behaviors that do not match the
expected. As I mentioned, {{NetgroupCache}} contents won't be defined.
* Performance considerations. Allocating "N" instances of {{Groups}} means
fetching the user's groups "N" times. Therefore, Guava cacheLoader's refresh
will be done "N" times, and so on.
Why did you decide to make that change instead of fixing the design of the unit
tests?
IIUC, there is a need to fix that bug in a follow up Jira.
> 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
> Fix For: 3.4.0
>
> Attachments: YARN-10425.001.patch, YARN-10425.002.patch,
> YARN-10425.003.patch, YARN-10425.004.patch, YARN-10425.005.patch,
> YARN-10425.006.patch, YARN-10425.007.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
[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=17229219#comment-17229219 ] Peter Bacsko edited comment on YARN-10425 at 11/10/20, 1:41 PM: "ret represents return value, I think it's quite expressive, since the function's name and description already define what kind of data we store there it's more important to let the reader know, we are setting the return value" Did some research on this before posting, turns out there are people who indeed use "ret". I'd still be more comfortable with a name like "result", though (https://softwareengineering.stackexchange.com/questions/134321/is-it-a-good-practice-to-name-the-returned-variable-result) "_There are only two hard things in Computer Science: cache invalidation and_ _naming things.__”_ was (Author: pbacsko): "ret represents return value, I think it's quite expressive, since the function's name and description already define what kind of data we store there it's more important to let the reader know, we are setting the return value" Did some research on this before posting, turns out there are people who indeed use "ret". I'd still be more comfortable with a name like "result", though. "_There are only two hard things in Computer Science: cache invalidation and_ _naming things.__”_ > 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, YARN-10425.002.patch, > YARN-10425.003.patch, YARN-10425.004.patch, YARN-10425.005.patch, > YARN-10425.006.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
[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=17229219#comment-17229219 ] Peter Bacsko edited comment on YARN-10425 at 11/10/20, 1:39 PM: "ret represents return value, I think it's quite expressive, since the function's name and description already define what kind of data we store there it's more important to let the reader know, we are setting the return value" Did some research on this before posting, turns out there are people who indeed use "ret". I'd still be more comfortable with a name like "result", though. "_There are only two hard things in Computer Science: cache invalidation and_ _naming things.__”_ was (Author: pbacsko): "ret represents return value, I think it's quite expressive, since the function's name and description already define what kind of data we store there it's more important to let the reader know, we are setting the return value" Did some research on this before posting, turns out there are people who indeed use "ret". I'd still be more comfortable with a name like "result", though. "_There are only two hard things in Computer Science: cache invalidation and_ _naming things._*_”_* > 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, YARN-10425.002.patch, > YARN-10425.003.patch, YARN-10425.004.patch, YARN-10425.005.patch, > YARN-10425.006.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
[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=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
