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 CpuResourceHandler.
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, IMO.
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.