[
https://issues.apache.org/jira/browse/YARN-10528?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17252054#comment-17252054
]
Szilard Nemeth commented on YARN-10528:
---------------------------------------
Thanks [~sahuja] fo working on this, very good work as I said before, good
explanation + thorough manual testing evidence.
Checked your patch, I only found one little "mistake".
Regarding the 2 added testcases:
TestAllocationFileLoaderService#testParentTagWithMaxAMShare and
TestAllocationFileLoaderService#testParentWithMaxAMShare
Let's consider the try-catch block:
{code}
try {
allocLoader.reloadAllocations();
} catch (AllocationConfigurationException ex) {
assertEquals(ex.getMessage(), "The configuration settings for root.parent"
+ " are invalid. A queue element that contains child queue elements"
+ " or that has the type='parent' attribute cannot also include a"
+ " maxAMShare element.");
}
{code}
There are 3 ways I'm aware of to assert thrown exceptions + their messages:
2 out of 3 is described on this page:
https://www.baeldung.com/junit-assert-exception#junit-4
1. Either using the Test annotation's expected field
2. Use the ExpectedException rule class
3. Use the methodology that you applied: Catch and check the message.
Probably your method is the most natural and straightforward.
Howeve, you need to add a fail("some message") statement as the last statement
of the try block (after reloadAllocations), as if the production code does not
throw an exception, the test will still pass as no assertion statement would be
run in that case.
Apart from this, the patch looked good to me.
As this is a bugfix, did you consider to backport this to branches: branch-3.3,
branch-3.2, branch-3.1?
I think it's worth to backport this bugfix until 3.1
Thanks
> maxAMShare should only be accepted for leaf queues, not parent queues
> ---------------------------------------------------------------------
>
> Key: YARN-10528
> URL: https://issues.apache.org/jira/browse/YARN-10528
> Project: Hadoop YARN
> Issue Type: Bug
> Reporter: Siddharth Ahuja
> Assignee: Siddharth Ahuja
> Priority: Major
> Attachments: YARN-10528.001.patch, maxAMShare for root.users (parent
> queue) has no effect as child queue does not inherit it.png
>
>
> Based on [Hadoop
> documentation|https://hadoop.apache.org/docs/current/hadoop-yarn/hadoop-yarn-site/FairScheduler.html],
> it is clear that {{maxAMShare}} property can only be used for *leaf queues*.
> This is similar to the {{reservation}} setting.
> However, existing code only ensures that the reservation setting is not
> accepted for "parent" queues (see
> https://github.com/apache/hadoop/blob/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/allocation/AllocationFileQueueParser.java#L226
> and
> https://github.com/apache/hadoop/blob/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/allocation/AllocationFileQueueParser.java#L233)
> but it is missing the checks for {{maxAMShare}}. Due to this, it is
> currently possible to have an allocation similar to below:
> {code}
> <?xml version="1.0" encoding="UTF-8" standalone="yes"?>
> <allocations>
> <queue name="root">
> <weight>1.0</weight>
> <schedulingPolicy>drf</schedulingPolicy>
> <aclSubmitApps>*</aclSubmitApps>
> <aclAdministerApps>*</aclAdministerApps>
> <queue name="default">
> <weight>1.0</weight>
> <schedulingPolicy>drf</schedulingPolicy>
> </queue>
> <queue name="users" type="parent">
> <weight>1.0</weight>
> <schedulingPolicy>drf</schedulingPolicy>
> <maxAMShare>1.0</maxAMShare>
> </queue>
> </queue>
> <defaultQueueSchedulingPolicy>fair</defaultQueueSchedulingPolicy>
> <queuePlacementPolicy>
> <rule name="specified" create="true"/>
> <rule name="nestedUserQueue" create="true">
> <rule name="default" create="true" queue="users"/>
> </rule>
> <rule name="default"/>
> </queuePlacementPolicy>
> </allocations>
> {code}
> where {{maxAMShare}} is 1.0f meaning, it is possible allocate 100% of the
> queue's resources for Application Masters. Notice above that root.users is a
> parent queue, however, it still gladly accepts {{maxAMShare}}. This is
> contrary to the documentation and in fact, it is very misleading because the
> child queues like root.users.<user> actually do not inherit this setting at
> all and they still go on and use the default of 0.5 instead of 1.0, see the
> attached screenshot as an example.
--
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
