[jira] [Commented] (YARN-10277) CapacityScheduler test TestUserGroupMappingPlacementRule should build proper hierarchy

["Gergely Pollak (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/YARN-10277?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17139459#comment-17139459 ] 

Gergely Pollak commented on YARN-10277:
---------------------------------------

Thank you for the path [~snemeth], I really like the compact declaration of queue hierarchy it actually exceeds my expectation, very nice builder there. Altogether the patch seems very nice, my only issue with it is we don't separate managed parents and parents enough. Let me explain:

In the build method you add all managedParents queues if those are not already present in the queues map, however those queues must not be present there, since the only way a queue can get to queuePaths list is if it was added as a parent queue, which means this queue is declared both parent AND managedParent, this should result in an illegal state exception in my opinion.

The other place where we mix the two types of parents together is the addQueues method, which creates all the required parent queues if those were not specified yet (I really love this feature btw), however if I declare a queue as a managed parent, this method will accept it as a valid parent for it's queue. So if I have a root.managed queue as a managed parent, I will still be able to add the root.managed.something.deep queue, since the root.managed will be accepted by the addQueues as a valid parent for a non-auto created queue. Which is an invalid state according to the documentation:
{code:java}
The Dynamic Queue Auto-Creation and Management feature is integrated with the CapacityScheduler queue hierarchy and can be configured for a ParentQueue currently to auto-create leaf queues. Such parent queues do not support other pre-configured queues to co-exist along with auto-created queues.
{code}
While I know this tool is just to setup test cases, and we can tell the coders not to setup invalid trees (implementing these checks is somewhat implementing business logic in tests), but I think it can lead to some misunderstanding on the developer side if we hide the issues it might cause.

> CapacityScheduler test TestUserGroupMappingPlacementRule should build proper hierarchy
> --------------------------------------------------------------------------------------
>
>                 Key: YARN-10277
>                 URL: https://issues.apache.org/jira/browse/YARN-10277
>             Project: Hadoop YARN
>          Issue Type: Task
>            Reporter: Gergely Pollak
>            Assignee: Szilard Nemeth
>            Priority: Major
>         Attachments: YARN-10277.001.patch
>
>
> Since the CapacityScheduler internal implementation depends more and more on queue being hierarchical, the test gets really hard to maintain. A lot of test cases were failing because they used non existing queues, but the older placement rule solution ignored missing parents, but since the leaf queue change in CS, we must be able to get a full path for any queue, since all queues are referenced by their full path.
> This test should reflect this and instead of creating and expecting the existance of fictional queues, it should create a proper queue hierarchy, with a way to describe it better. 
> Currently we set up a bunch of mockito "when" statements to simulate the queue behavior, but this is a hassle to maintain, and easy to miss a few method.



--
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