[jira] [Commented] (YARN-1655) Add implementations to FairScheduler to support increase/decrease container resource

["Szilard Nemeth (JIRA)" <ji...@apache.org>]

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

Szilard Nemeth commented on YARN-1655:
--------------------------------------

Hi [~wilfreds]!

Thanks for this patch!
I reviewed the code and the changes look good to me in the production code.
I think the test code is straightforward to read (thanks for the comment inside testcases) and is of high quality.
Please see my comments to the test code (TestContainerResizing):


1. In the javadoc of TestContainerResizing#testSimpleIncreaseContainer: "Add a increase" should be "Add an increase".
2. There's a method called "sentRMContainerLaunched" in this class. I don't understand its name. As far as I understand, this method just that a LAUNCHED type of event is handled by the container state machine. Shouldn't the name be "setRMContainerLaunched"? 
3. There's a comment used 7 times throughout the class: "Acquire them, and NM report RUNNING". Can you use a more descriptive comment instead of this? 
4. For more readable code and clarity, I would wrap appMaster.allocate calls behind well-named methods, as they are mainly called in 2 forms: 
The first: 
{code:java}
appMaster.allocate(Collections.singletonList(
 ResourceRequest.newInstance(Priority.newInstance(1), "*",
 Resources.createResource(2 * GB), 1)), null); {code}

So: Priority is 1, host is "*" and there's a resource argument which is the only important part of the call.

The second:
{code:java}
appMaster.allocate(null, null){code}
The latter form is hard to understand and the readers of the testcode need to delve into the production code to understand why the parameters are null, that is why I think it could be refactored like I suggested.

5. Javadoc of method testOrderOfIncrease starts with: "There're multiple containers need to be increased.". I think it can be rephrased to "Multiple containers need to be increased" or something like this.
6. In method testOrderOfIncrease: There are multiple occurrences of statement: "1 * GB", this could be simplified to "GB".

7. In the debug message: 
{code:java}
LOG.debug("Checked pending queue resources: used " + used + " demand "
 + demand);{code}
--> There's a missing comma between the values.

8. For better troubleshooting of potential issues, I think the failure message need to be updated in method sentRMContainerLaunched, as it does not print the containerId.

> Add implementations to FairScheduler to support increase/decrease container resource
> ------------------------------------------------------------------------------------
>
>                 Key: YARN-1655
>                 URL: https://issues.apache.org/jira/browse/YARN-1655
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager, scheduler
>            Reporter: Wangda Tan
>            Assignee: Wilfred Spiegelenburg
>            Priority: Major
>         Attachments: YARN-1655.001.patch, YARN-1655.002.patch, YARN-1655.003.patch
>
>




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