Sidharta Seethana commented on YARN-3542:

Hi Vinod, 

A few comments inline regarding your review comments.


When user sets the old CgroupsLCEResourcesHandler, you are internally resetting 
it to DefaultLCEResourcesHandler(inside LinuxContainerExecutor) and using that 
as a control to stop using the older handler. This effectively means the old 
code is not used anymore, and that the new code is stable. This doesn't map 
well with the earlier decision to not document the new handlers yet.

Thats a good point. However, the handlers themselves aren’t the issue IMO, but 
rather the configuration which might get out of hand as we add more resource 
handlers. This is especially true in light of resource profiles being worked in 
in YARN-3926 - which might require some changes to how the resource handlers 
are configured.  However, there should be no issue hooking into the new handler 
using the old configuration mechanism. 

Lot more dedup is possible between the new hierarchy and the older hierarchy

I don’t think we should dedup this code. There is no reason to hook into the 
new code/handlers for either of the two scenarios being discussed here : 1) 
deprecate the old handler in which case there shouldn’t be any reason to make 
make major changes to it. 2) not deprecate the old handler because the new 
handler might not be stable - in which case it doesn’t make sense to hook into 
the new handler/code yet. 

Further, specific constants like CPU_PERIOD_US perhaps belong better to the 
specific implementations likeCGroupsCpuResourceHandlerImpl instead of the root 
I am assuming you meant this :  ‘the root CGroupsHandler’ not ‘the root 
CpuResourceHandler’.  Arguments can be made both ways here : CGroupsHandler 
already has enums and constants that are used across multiple resource handler 
implementations. Some of these cannot be moved out (e.g the enum, tasks etc) . 
Moving out some of these to individual handlers is of limited use and makes it 
hard to get an quick overview of all the cgroups subsystems/parameters in use 
among the various resource handlers. It also creates problems if new handlers 
for the same subsystem are created which require using the same cgroup 
parameters. Cleaning this up fully would require more extensive refactoring - 
individual classes for various cgroups subsystems etc., This is not necessary, 

We should also deprecate the old LCEResourcesHandler interface, 
DefaultLCEResourcesHandler etc. That said, we shouldn't deprecate them or 
CgroupsLCEResourcesHandler before we make the newer mechanism public and 
usable. So may be we should fork off all this deprecation to another JIRA and 
only get to it after we publicly document the new mechanism for stable usage.

Yeah, thats a good point again. The new handler itself isn’t the issue - the 
new configuration could be. Like I said before, though, I think there should be 
no problem using the new handler if the old configuration mechanism is in use. 

> Re-factor support for CPU as a resource using the new ResourceHandler 
> mechanism
> -------------------------------------------------------------------------------
>                 Key: YARN-3542
>                 URL: https://issues.apache.org/jira/browse/YARN-3542
>             Project: Hadoop YARN
>          Issue Type: Bug
>          Components: nodemanager
>            Reporter: Sidharta Seethana
>            Assignee: Varun Vasudev
>            Priority: Critical
>         Attachments: YARN-3542.001.patch, YARN-3542.002.patch, 
> YARN-3542.003.patch, YARN-3542.004.patch, YARN-3542.005.patch, 
> YARN-3542.006.patch
> In YARN-3443 , a new ResourceHandler mechanism was added which enabled easier 
> addition of new resource types in the nodemanager (this was used for network 
> as a resource - See YARN-2140 ). We should refactor the existing CPU 
> implementation ( LinuxContainerExecutor/CgroupsLCEResourcesHandler ) using 
> the new ResourceHandler mechanism. 

This message was sent by Atlassian JIRA

Reply via email to