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