[
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: [email protected]
For additional commands, e-mail: [email protected]
