[
https://issues.apache.org/jira/browse/YARN-3542?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15065910#comment-15065910
]
Sidharta Seethana commented on YARN-3542:
-----------------------------------------
Hi Vinod,
A few comments inline regarding your review comments.
Thanks,
-Sidharta
{quote}
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.
{quote}
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.
{quote}
Lot more dedup is possible between the new hierarchy and the older hierarchy
{quote}
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.
{quote}
Further, specific constants like CPU_PERIOD_US perhaps belong better to the
specific implementations likeCGroupsCpuResourceHandlerImpl instead of the root
CpuResourceHandler.
{quote}
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.
{quote}
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.
{quote}
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
(v6.3.4#6332)
