[
https://issues.apache.org/jira/browse/YARN-8016?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16396474#comment-16396474
]
Wangda Tan edited comment on YARN-8016 at 3/13/18 3:11 AM:
-----------------------------------------------------------
Thanks [~Zian Chen] for working on the patch.
1) updatePlacementRules, why add readLock? It is already under writelock
protection.
2) why check this:
{code:java}
if (null != rule.getQueueMappingLists()) {
placementRules.add(rule);
}
{code}
Instead of
{code:java}
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:java}
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:java}
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)
was (Author: leftnoteasy):
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
