[
https://issues.apache.org/jira/browse/YARN-6445?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15963457#comment-15963457
]
Daniel Templeton commented on YARN-6445:
----------------------------------------
Thanks for the patch, [~vvasudev]. A few comments:
* {{TestResources}}
** I don't see why we need the {{ExtendedResources}} class. You don't use the
{{none()}} method that I see, and I don't see where the {{unbounded()}} method
is materially different from what's in {{Resources}}.
* {{DominantResourceCalculator}}
** You left in some commented-out code: {code} // ResourceInformation
resourceInformation = ResourceInformation
// .newInstance(numerator.getResourceInformation(resource));{code}
** I found this logic in {{divideAndCeil()}} to be surprisingly obtuse: {code}
ResourceInformation resourceInformation =
ret.getResourceInformation(resource);
ResourceInformation.copy(numerator.getResourceInformation(resource),
resourceInformation);{code} It would really help to add a comment
that explains what's happening.
** You left in more commented-out code: {code} // ResourceInformation
tmp =
// ResourceInformation.newInstance(rResourceInformation);{code}
** You left in commented-out code: {code} // tmp.setValue(value);
ResourceInformation
.copy(rResourceInformation, ret.getResourceInformation(resource));
ret.getResourceInformation(resource).setValue(value);
// ret.setResourceInformation(resource, tmp);{code}
* {{Resource}}
** In {{newInstance()}} you have this: {code} public static Resource
newInstance(Resource resource) {
Resource ret = Resource.newInstance(0, 0);
for (Map.Entry<String, ResourceInformation> entry : resource.getResources()
.entrySet()) {
try {
ResourceInformation
.copy(entry.getValue(), ret.getResourceInformation(entry.getKey()));
} catch (YarnException ye) {
ret.setResourceInformation(entry.getKey(),
ResourceInformation.newInstance(entry.getValue()));
}
}
return ret;
}{code} Since you know that {{ret}} only has CPU and memory, this code seems
odd to me. Maybe add a comment to be clear about what you're doing so no one's
confused.
* {{ResourcePMImpl}}
** In {{getResources()}}, I don't see the reason to remove the unmodifiable
wrapper.
* In general, I'm finding the idiom of getting the RI into a tmp object and
then copying the RI from the source into the tmp RI to be obtuse. Can you add
a wrapper method that does the same thing but labels it with an obvious name?
Or maybe just do what you do is some places: {code} ResourceInformation
.copy(rResourceInformation,
ret.getResourceInformation(resource));{code}
* There are a lot of places where {{Resource.getResourceInformation()}} is
called in context of a _try-catch_ that just wraps the exception in an
{{IllegalArgumentException}} and rethrows it. It's out of scope for this JIRA,
but I think that's the wrong thing to do. The correct approach is what you do
in {{Resource.newInstance()}}, i.e. treat it like what it is: a missing
resource, and move on.
> Improve YARN-3926 performance with respect to SLS
> -------------------------------------------------
>
> Key: YARN-6445
> URL: https://issues.apache.org/jira/browse/YARN-6445
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager, resourcemanager
> Reporter: Varun Vasudev
> Assignee: Varun Vasudev
> Attachments: YARN-6445-YARN-3926.001.patch,
> YARN-6445-YARN-3926.002.patch
>
>
> As part of the SLS runs on YARN-3926, we discovered a bunch of bottlenecks
> around object creation and garbage collection. This JIRA is to apply a fix
> for those bottlenecks.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
