[
https://issues.apache.org/jira/browse/YARN-6504?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16003617#comment-16003617
]
Daniel Templeton commented on YARN-6504:
----------------------------------------
A few comments, [~vvasudev]:
{{TaskAttemptImpl}}
* Since you're setting {{resourceProfile}} in the constructor, it would be
better not to set it in the declaration.
* The {{LOG.info()}} in the constructor should probably be {{LOG.debug()}}. I
might also move it into {{getResourceProfile()}}.
{{ContainerRequestEvent}}
* {{Configuration}} import is unused.
* {{resourceProfile}} in the constructor args should probably come right after
{{capability}}.
* Is it useful to overload {{createContainerRequestEventForFailedContainer()}}?
Doesn't look like the 2-arg version is needed anymore. And if you dump the
2-arg version, you can add profile to the 2-arg constructor, making
{{createContainerRequestEventForFailedContainer()}} simpler.
* Missing javadoc for new accessors.
{{RMCommunicator}}
* Missing javadoc for {{getResourceProfilesMap()}}
{{RMContainerAllocator}}
* {{Hamlet}} import is unused.
* Might be cleaner to move the logic about calculating a resource from the
profile and capability into a method you can reuse.
{{RMContainerRequester}}
* The profile arg in the {{ContainerRequest}} constructors should come right
after capability.
{{MRJobConfig}}
* {{DEFAULT_REDUCE_RESOURCE_PROFILE}} appears unused.
{{ProfileCapability}}
* Is it important to fail a null override? I should think it would be
friendlier to treat it as {{Resource.newInstance(0, 0)}}.
* In {{toResource()}} returning the override if the profile map in empty seems
a nonintuitive choice. Why not return the default profile? In any case, the
javadoc should explain the expected return values for all the special cases.
* {{none}} in {{toResource()}} should be a constant.
* In {{toResource()}} the consecutive _if_s in the _for_ loop can be combined.
Considering that there could be a large number of resource types, it probably
makes more sense to scrap the loop for an _if-memory_ and an _if-cpu_.
{{Resource}}
* In {{newInstance()}} the _try-catch_ doesn't cover all cases.
{{ResourcePBImpl.getResourceInformation()}} throws a
{{ResourceNotFoundException}}, which is not a {{YarnException}}.
{{TestResourceProfiles}}
* In {{testConvertProfileToResourceCapability()}}, the _try_ should start right
before the copy.
> Add support for resource profiles in MapReduce
> ----------------------------------------------
>
> Key: YARN-6504
> URL: https://issues.apache.org/jira/browse/YARN-6504
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager, resourcemanager
> Reporter: Varun Vasudev
> Assignee: Varun Vasudev
> Attachments: YARN-6504-YARN-3926.001.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
