[jira] [Commented] (YARN-6504) Add support for resource profiles in MapReduce

["Daniel Templeton (JIRA)" <ji...@apache.org>]

    [ 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: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org