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

Junping Du commented on YARN-3366:
----------------------------------

Thanks [~sidharta-s] for delivering the patch! A few comments so far:
In YarnConfiguration.java,
{code}
+  /** This setting controls if resource handling for network bandwidth is 
enabled **/
+  /* Work in progress: This configuration parameter may be changed/removed in 
the future */
+  @Private
+  public static final String NM_NETWORK_RESOURCE_ENABLED =
+      NM_NETWORK_RESOURCE_PREFIX + "enabled";
+  /** Network as a resource is disabled by default **/
+  public static final boolean DEFAULT_NM_NETWORK_RESOURCE_ENABLED = false;
{code}
Why we are explicitly saying "Work in progress: This configuration parameter 
may be changed/removed in the future" here and in other places? These 
configuration properties, once go in and released, can only deprecated but 
cannot removed without major release update.

{code}
+  public static final String NM_NETWORK_RESOURCE_INTERFACE =
+      NM_NETWORK_RESOURCE_PREFIX + "interface";
+  public static final String DEFAULT_NM_NETWORK_RESOURCE_INTERFACE = "eth0";
{code}
Shall we support the case for multiple network interfaces? I know the user can 
do something like nic teaming in OS configuration layer. However, YARN 
shouldn't assume user have to do this. Isn't it? If so, better to update to 
String array.

In LinuxContainerExecutor.java,
{code}
+    try {
+      resourceHandlerChain = ResourceHandlerModule
+          .getConfiguredResourceHandlerChain(super.getConf());
+      if (resourceHandlerChain != null) {
+        resourceHandlerChain.bootstrap(super.getConf());
+      }
+    } catch (ResourceHandlerException e) {
+      LOG.error("Failed to bootstrap configured resource subsystems! ", e);
+      throw new IOException("Failed to bootstrap configured resource 
subsystems!");
+    }
{code}
If "NM_NETWORK_RESOURCE_ENABLED" = false, the resourceHandlerChain will still 
be initiated with empty handler but not null. So 
resourceHandlerChain.bootstrap() still get called which is not necessary and 
possible get exception thrown out. I think we should make sure we don't involve 
any operations if all resources configuration are disabled (NETWORK_RESOURCE 
only so far).
In this case, may be we should make resourceHandlerChain to be null when 
NM_NETWORK_RESOURCE_ENABLED is false (assume other resources haven't onboard so 
far)? Also, other operations like: postComplete, reacquireContainer, etc. has 
the same issue.

{code}
+          for (PrivilegedOperation op : ops) {
+            switch (op.getOperationType()) {
+              case ADD_PID_TO_CGROUP:
+                resourceOps.add(op);
+                break;
+              case TC_MODIFY_STATE:
+                tcCommandFile = op.getArguments().get(0);
+              default:
+                LOG.warn("PrivilegedOperation type unsupported in launch: "
+                    + op.getOperationType());
+                continue;
+            }
+          }
{code}
We miss a break in case TC_MODIFY_STATE: ?

In ResourceHandlerModule.java,
{code}
+    if (cGroupsHandler == null) {
+      synchronized (CGroupsHandler.class) {
+        cGroupsHandler = new CGroupsHandlerImpl(conf,
+            PrivilegedOperationExecutor.getInstance(conf));
+      }
+    }
{code}
We miss a null check again inside of the synchronized (CGroupsHandler.class) 
block.


{code}
+  private static boolean addHandlerIfNotNull(List<ResourceHandler> handlerList,
+      ResourceHandler handler) {
+    return (handler != null) && handlerList.add(handler);
+  }
{code}
Return a boolean value is not necessary?

In TrafficControlBandwidthHandlerImpl.java,
{code}
+  //In the absence of 'scheduling' support, we'll 'infer' the guaranteed
+  //outbound bandwidth for each container based on this number. This will
+  //likely go away once we add support on the RM for this resource type.
+  private static final int MAX_CONTAINER_COUNT = 100;
...
+    containerBandwidthMbit = (int) Math.ceil((double) yarnBandwidthMbit /
+        MAX_CONTAINER_COUNT);
{code}
Can we make containerBandwidthMbit being calculated dynamically by number of 
containers running on NM? If not, setting MAX_CONTAINER_COUNT to 100 sounds too 
large to me which could make containerBandwidthMbit smaller than necessary. 
Typically, containers on a powerful machine in production environment should be 
10 - 50, may be we could set something like: 50 here? 

For postComplete(ContainerId containerId), we should return op instead of null?

In TrafficController.java,
{code}
+      if (LOG.isDebugEnabled()) {
+        LOG.debug("TC state: ");
+        LOG.debug(output);
+      }
...
+      if (LOG.isDebugEnabled()) {
+        LOG.debug("classId -> bytes sent");
+        LOG.debug(classIdBytesStats);
+      }
{code}
Can we merge two LOG.debug() into one line with %n if we want a new line in 
log? There are many other places need to be fixed too.



> Outbound network bandwidth : classify/shape traffic originating from YARN 
> containers
> ------------------------------------------------------------------------------------
>
>                 Key: YARN-3366
>                 URL: https://issues.apache.org/jira/browse/YARN-3366
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Sidharta Seethana
>            Assignee: Sidharta Seethana
>         Attachments: YARN-3366.001.patch, YARN-3366.002.patch, 
> YARN-3366.003.patch, YARN-3366.004.patch, YARN-3366.005.patch
>
>
> In order to be able to isolate based on/enforce outbound traffic bandwidth 
> limits, we need  a mechanism to classify/shape network traffic in the 
> nodemanager. For more information on the design, please see the attached 
> design document in the parent JIRA.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to