[
https://issues.apache.org/jira/browse/YARN-6445?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Varun Vasudev updated YARN-6445:
--------------------------------
Attachment: YARN-6445-YARN-3926.003.patch
Thanks for the review [~templedf]!
bq. 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.
The tests in TestResources run with different resource types. This combined
with the fact that Resources.NONE and Resources.UNBOUNDED are final static
variables means that depending on the ordering of the tests, Resources.NONE and
Resources.UNBOUNDED may or may not have the entries for all the resource types.
The ExtendedResources class just lets me create a new FixedValueResource every
time.
{quote}
You left in some commented-out code:
// ResourceInformation resourceInformation = ResourceInformation
// .newInstance(numerator.getResourceInformation(resource));
// ResourceInformation tmp =
// ResourceInformation.newInstance(rResourceInformation);
You left in commented-out code:
// tmp.setValue(value);
ResourceInformation
.copy(rResourceInformation,
ret.getResourceInformation(resource));
ret.getResourceInformation(resource).setValue(value);
// ret.setResourceInformation(resource, tmp);
{quote}
Doh! Removed all of these.
{quote}
I found this logic in divideAndCeil() to be surprisingly obtuse:
ResourceInformation resourceInformation =
ret.getResourceInformation(resource);
ResourceInformation.copy(numerator.getResourceInformation(resource),
resourceInformation);
It would really help to add a comment that explains what's happening.
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:
ResourceInformation
.copy(rResourceInformation,
ret.getResourceInformation(resource));
{quote}
Fair point. I've removed the copy calls and cleaned up the code a bit.
{quote}
Resource
In newInstance() you have this:
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;
}
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.
{quote}
ret doesn't have only memory and cpu - it will have as many resources as are
setup on the RM. However, this code also got cleaned up as part of the copy
clean up. Let me know if it still needs a comment.
{quote}
ResourcePMImpl
In getResources(), I don't see the reason to remove the unmodifiable
wrapper.
{quote}
Couple of reasons for this - 1. It was creating a lot of objects that were
created and then never used. The API is inconsistent - I can get the
ResourceInformation for a specify type and modify it but I can't get the
ResourceInformation for all the types to modify them.
{quote}
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.
{quote}
Hmm - let me think about this and get back to you.
> 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, YARN-6445-YARN-3926.003.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: [email protected]
For additional commands, e-mail: [email protected]
