hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Karthik Kambatla (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-2883) Queuing of container requests in the NM
Date Wed, 13 Apr 2016 16:00:28 GMT

    [ https://issues.apache.org/jira/browse/YARN-2883?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15239499#comment-15239499

Karthik Kambatla commented on YARN-2883:

Discussed this offline with [~asuresh], [~kkaranasos] and [~chris.douglas]. Regarding queuing
vs immediately starting guaranteed containers, it is reasonable to queue it as part of YARN-2883.
In YARN-1011, we could add the option of starting them directly or using the actual utilization
for determining resource availability. 

The logic sounds good to me. Most of my comments are cosmetic (readability/maintainability)
in nature. Since this change will only be in trunk (and not branch-2), I am comfortable with
checking this in to unblock other efforts. I am open to addressing some of my comments in
a subsequent JIRA, and happy to contribute those changes too. 

# QueuingContainerManagerImpl
## I see that additions to opportContainersToKill are in the monitor. My latter comments recommend
moving the helper methods in monitor to manager itself. If we don't do that, it would help
to add a comment mentioning where this set is populated. 
## Nit: Rename opportContainersToKill to opportunisticContainersToKill? 
## Nits: In the constructor, don't need to specify the type of ConcurrentLinkedQueue
## Nit: The following code could be merged into a single line:
      if (allocatedContInfo
          .getExecutionType() == ExecutionType.GUARANTEED) {
## Nit: Rename killOpportContainers to killOpportunisticContainers
## Would it make sense to have a field for queuingContainerMonitor since it is used at several
places? I am open to calling this containersMonitor depending on whether we choose to relax
the visibility of the same name field in the parent class.
# QueuingContainersMonitorImpl
## Several methods and fields seem to use utilization/usage in their names; this is misleading
as it gives the impression we are looking at the utilization instead of allocation/limit.
## Would it make sense to track the aggregateContainerAllocation using ProcessTreeInfo instead
of ResourceUtilization? The latter likely works better for YARN-1011, but I am fine with either.

## Would it make sense to track the aggregateContainerAllocation in ContainersMonitorImpl
itself? This can be updated when we add/remove items from trackingContainers? That way, most
of the helper methods in QueuingContainersMonitorImpl can just move to QueuingContainerManager,
and the helper methods themselves will not need all the parameters they are passed making
the code more readable.
## Nit: Rename opportContainersToKill to opportunisticContainersToKill and even more preferably
to identifyOpportunisticContainersToKill? 
# ContainerImpl: The second change appears spurious. 
# ContainersMonitorImpl
## Visibility of some of the fields has been relaxed. Not all of them are required. Some of
them are for tests; can we add @VisibleForTesting along with a comment about what the visibility
could be if it weren't for the tests?
## Observation: The addition of onStart etc. methods is not necessary, but makes the code
easier to understand. 
# Should the config being added be a queue length with a default of 0 instead of a boolean
that we have now? I am fine with filing a follow-up to fix this up. My intent here is to limit
the new configs we add and avoid redundancy.
# TestQueuingContainerManager
## createContainerManager defines getRemoteUgi the exact same way as TestContainerManager.
Any chance we can avoid the duplication? 
## Would it make sense to define the right hasResources in the setup method itself? 
## When creating a new ArrayList, don't need to specify the type. 

> Queuing of container requests in the NM
> ---------------------------------------
>                 Key: YARN-2883
>                 URL: https://issues.apache.org/jira/browse/YARN-2883
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager, resourcemanager
>            Reporter: Konstantinos Karanasos
>            Assignee: Konstantinos Karanasos
>         Attachments: YARN-2883-trunk.004.patch, YARN-2883-trunk.005.patch, YARN-2883-trunk.006.patch,
YARN-2883-trunk.007.patch, YARN-2883-trunk.008.patch, YARN-2883-trunk.009.patch, YARN-2883-trunk.010.patch,
YARN-2883-trunk.011.patch, YARN-2883-yarn-2877.001.patch, YARN-2883-yarn-2877.002.patch, YARN-2883-yarn-2877.003.patch,
> We propose to add a queue in each NM, where queueable container requests can be held.
> Based on the available resources in the node and the containers in the queue, the NM
will decide when to allow the execution of a queued container.
> In order to ensure the instantaneous start of a guaranteed-start container, the NM may
decide to pre-empt/kill running queueable containers.

This message was sent by Atlassian JIRA

View raw message