[
https://issues.apache.org/jira/browse/YARN-9298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16776175#comment-16776175
]
Yufei Gu edited comment on YARN-9298 at 2/24/19 9:19 AM:
---------------------------------------------------------
Hi [~wilfreds], thanks for the patch. It is really nice to add these unit
tests. Some comments:
1. Thanks for adding comments for method {{getPlacementRule(String ruleStr,
Configuration conf)}}, but it is only used by CS. You may need to update the
comments.
2. I would suggest the unit test messages to clarify the expectation or some
actions failed, like this “Rule object shouldn’t be null” or "Failed to
instantiate the rule object.". “Cleaned name was changed for clean input" could
be something like “Unexpected cleaned name.” Or “Failed to clean name”
3. Can you add a case “root” in method {{testAssureRoot()}}?
4. I feel like class {{TestPlacementRuleFS}} isn’t necessary. Why not just test
against DefaultPlacementRule, and all other real rules? Besides, Unit tests are
needed for the all FS placement rule classes. I’m OK if you want to move some
code from YARN-8967 and reuse existing tests, like the one in class
TestQueuePlacementPolicy
5. if {} else if {} else {} or a switch statement could be cleaner than if {}
else { if {} else {}} in method {{setConfig}}
6. There are some common code in method {{*Rule::initialize()}} and
{{*Rule::setConfig()}}, we can probably put them into either class
{{PlacementRule}} or class {{FairQueuePlacementUtils}}.
was (Author: yufeigu):
Hi [~wilfreds], thanks for the patch. It is really nice to add these unit
tests. Some comments:
1. Thanks for adding comments for method {{getPlacementRule(String ruleStr,
Configuration conf)}}, but it is only used by CS. You may need to update the
comments.
2. I would suggest the unit test messages to clarify the expectation or some
actions failed, like this “Rule object shouldn’t be null”. “Cleaned name was
changed for clean input" could be something like “Unexpected cleaned name.” Or
“Failed to clean name”
3. Can you add a case “root” in method {{testAssureRoot()}}?
4. I feel like class {{TestPlacementRuleFS}} isn’t necessary. Why not just test
against DefaultPlacementRule, and all other real rules? Besides, Unit tests are
needed for the all FS placement rule classes. I’m OK if you want to move some
code from YARN-8967 and reuse existing tests, like the one in class
TestQueuePlacementPolicy
5. if {} else if {} else {} or a switch statement could be cleaner than if {}
else { if {} else {}} in method {{setConfig}}
6. There are some common code in method {{*Rule::initialize()}} and
{{*Rule::setConfig()}}, we can probably put them into either class
{{PlacementRule}} or class {{FairQueuePlacementUtils}}.
> Implement FS placement rules using PlacementRule interface
> ----------------------------------------------------------
>
> Key: YARN-9298
> URL: https://issues.apache.org/jira/browse/YARN-9298
> Project: Hadoop YARN
> Issue Type: Improvement
> Components: scheduler
> Reporter: Wilfred Spiegelenburg
> Assignee: Wilfred Spiegelenburg
> Priority: Major
> Attachments: YARN-9298.001.patch, YARN-9298.002.patch
>
>
> Implement existing placement rules of the FS using the PlacementRule
> interface.
> Preparation for YARN-8967
--
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
