hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel Templeton (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-6445) Improve YARN-3926 performance with respect to SLS
Date Mon, 10 Apr 2017 20:29:41 GMT

    [ 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 =
            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);
            .copy(rResourceInformation, ret.getResourceInformation(resource));
        // 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 {
            .copy(entry.getValue(), ret.getResourceInformation(entry.getKey()));
      } catch (YarnException ye) {
    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

To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org

View raw message