You are viewing a plain text version of this content. The canonical link for it is here.
Posted to yarn-issues@hadoop.apache.org by "Yufei Gu (JIRA)" <ji...@apache.org> on 2019/02/24 09:20:00 UTC
[jira] [Comment Edited] (YARN-9298) Implement FS placement rules
using PlacementRule interface
[ 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org