[ https://issues.apache.org/jira/browse/YARN-4166?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15981709#comment-15981709 ]
Naganarasimha G R commented on YARN-4166: ----------------------------------------- Thanks for the patch [~fly_in_gis], Overall approach seems to be fine and almost similar as what i intended to do, assigning this issue to you and will further help in getting it committed. Yes you were right that the findbugs issue is not related to the patch and have raised a separate issue(YARN-6515) for the same. Few comments on the patch: # ContainerExecutor ln no 183: {{updateContainerResource}} can be abstract like others and need not throw ResourceHandlerException. # ContainerExecutor ln no 183: {{ResourceHandlerException}} requires to be thrown for any other case ? May be captured in the javadocs on what scenarios this exception will be thrown.. # ContainerManagerImpl ln no 1238 : We need to invoke {{ContainerExecutor().updateContainerResource}}, this will ensure that on recovery proper resource information are assigned to containers else there is possibility that the resource update fails before cgroups modifications are done but on restart NM will report higher/lower cpu ... # CGroupsCpuResourceHandlerImpl ln no 246, invoking {{cGroupsHandler.deleteCGroup}} in setupLimits makes sense for the creating new container, but on update if there is excpetion just deleting the cgroup is not ideal and as well we might require to propagate the update failure to RM or fail the container, need to see how its handled (may be this error handling if not done can be done in other). # ResourceHandler ln no 72: IIUC no need to return List<PrivilegedOperations> as we are not taking any decision based on it. Here we just send null in most of the flows. IIUC PrivilegedOperation is used to squash such operations related optimization of container launch. And currently they support only for ADD_PID_TO_CGROUP, hence we can have the signature as void. And if required we can implement the stack properly when required. # May be once failure to update scenario handling is finalized we might need to add test case for the same. # can you please check whether the checkstyle is applicable to the patch ? > Support changing container cpu resource > --------------------------------------- > > Key: YARN-4166 > URL: https://issues.apache.org/jira/browse/YARN-4166 > Project: Hadoop YARN > Issue Type: Sub-task > Components: api, nodemanager, resourcemanager > Affects Versions: 2.8.0, 3.0.0-alpha2 > Reporter: Jian He > Assignee: Yang Wang > Attachments: YARN-4166.001.patch, YARN-4166.002.patch, > YARN-4166-branch2.8-001.patch > > > Memory resizing is now supported, we need to support the same for cpu. -- 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