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

Wangda Tan commented on YARN-8016:
----------------------------------

1) updatePlacementRules, why add readLock? It is already under writelock 
protection.

2) why check this: 
{code}
      if (null != rule.getQueueMappingLists()) {
        placementRules.add(rule);
      }
{code} 
Instead of 
{code}
      if (null != rule) {
        placementRules.add(rule);
      }
{code}
And I'm not sure why List<QueueMappingEntity> becomes a public interface, I 
think {{getPlacementForApp}} should be enough for mapping, no? 

3) IIUC, CapacitySchedulerConfiguration#getQueueMappingEntity is created for 
code reuse, but it's better not to place under CapacitySchedulerConfiguration. 
Maybe place it to a class like QueuePlacementRuleUtils under 
{{...resourcemanager.placement}}? 

4) Similarily, QueueMappingEntity is not a {{must-to-have}} field for a general 
PlacementRule, suggest to move it to a separate class instead of as a inner 
class of PlacementRule.

5)
{code}
public UserGroupMappingPlacementRule(){}
{code}
Is not necessary. 

6) {{validateParentQueue}} rename it to something like 
{{validateQueueMappingUnderParentQueue}} and place it under 
{{QueuePlacementRuleUtils}}?

7) AppNameMappingPlacementRule:
- Add a more detailed explanations about purpose, configs of this class to 
Javadocs? 
- Why {{getPlacementForApp}} returns empty?
- Basic test cases for this class?
- {{QUEUE_MAPPING_NAME}} maybe set it to {{app-name}} for short?

8) 
{{TestCapacitySchedulerAutoCreatedQueueBase#testUpdatePlacementRulesFactory}} 
should not belong to the {{TestCapacitySchedulerAutoCreatedQueueBase}}, the 
meaning of {{...Base}} is it contains non-static util functions and doesn't 
have have test cases. If you really want to reuse some of the methods, I 
suggest to extend the class and rename it to 
{{TestCapacitySchedulerQueueMappingBase}}. These test cases should be added to 
a class like {{TestCapacitySchedulerQueueMappingFactory}}. Following test cases 
can be considered: 
- Setup chain of placement rules. (I don't see a any test case include > 1 
placement rule). 

9) {{updatePlacementRules}}
- Add a {{app-name}} rule to switch .. case?
- For behavior of following statement:
{code}
    if (placementRuleStrs.isEmpty()) {
      PlacementRule ugRule = getUserGroupMappingPlacementRule();
      if (null != ugRule) {
        placementRules.add(ugRule);
      }
    } else {
      // ...
    }
{code} 
I think we should add getUserGroupMappingPlacementRule in any case, otherwise 
the {{yarn.scheduler.capacity.queue-mappings}} will be invalidated when 
{{YarnConfiguration.QUEUE_PLACEMENT_RULES}} has non-empty value which is a 
behavior change.
Instead this, I propose:
a. Check added rule class must be unique. (no two rule with same full qualified 
class name can be added). 
b. If UserGroupMappingPlacementRule is absent, add it to the tail of the list 
(and print log)

> Provide a common interface for queues mapping rules
> ---------------------------------------------------
>
>                 Key: YARN-8016
>                 URL: https://issues.apache.org/jira/browse/YARN-8016
>             Project: Hadoop YARN
>          Issue Type: Task
>            Reporter: Zian Chen
>            Assignee: Zian Chen
>            Priority: Major
>         Attachments: YARN-8016.001.patch
>
>
> Currently in Capacity Scheduler, we hard code queue mappings to 
> UserGroupMappingPlacementRule for queue mappings.
> We need to expose a general framework to dynamically create various queue 
> mapping placement rules by reading queue mapping rule property from 
> capacity-scheduler.xml   



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org

Reply via email to