hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikas Saha (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-771) AMRMClient support for resource blacklisting
Date Tue, 27 Aug 2013 17:59:53 GMT

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

Bikas Saha commented on YARN-771:
---------------------------------

Thanks for the patch. The overall approach looks good.

Comments on the patch itself.

"by adding or removing" or "with addition and removal"
{code}
Update application's black list with adding or removing resources.
{code}

This copying is unnecessary as newInstance does a copy anyways.
{code}
+        if (blacklistAdditions != null) {
+          blacklistToAdd = new ArrayList<String>(blacklistAdditions);
+        }
+        if (blacklistRemovals != null) {
+          blacklistToRemove = new ArrayList<String>(blacklistRemovals);
+        }
+        
+        ResourceBlacklistRequest blackListRequest = 
+            (blacklistToAdd != null) || (blacklistToRemove != null) ? 
+            ResourceBlacklistRequest.newInstance(blacklistToAdd,
+                blacklistToRemove) : null;
{code}

Can we please move these to different statements. More readable that way IMO.
{code}
+        blacklistAdditions = blacklistRemovals = null;
{code}

blacklistAdditions/Removals may have been updated at this point. So we cannot simply override
them. We need to append to those collections and also dedupe them if necessary. Will help
if they were Sets. Also, we could update the existing test in tesAMRMClient to verify this
behavior. There is an existing check for the ask list case.
{code}
+          if (blacklistToAdd != null) {
+            blacklistAdditions = new ArrayList<String>(blacklistToAdd);
+          }
+          if (blacklistToRemove != null) {
+            blacklistRemovals = new ArrayList<String>(blacklistToRemove);
+          }
{code}

Since we will be taking these from the user and then using them later on, does it make sense
to make a copy of the contents instead of holding onto user references?
{code}
+  public synchronized void updateBlackList(List<String> blacklistAdditions,
+      List<String> blacklistRemovals) {
+    this.blacklistAdditions = blacklistAdditions;
+    this.blacklistRemovals = blacklistRemovals;
{code}
                
> AMRMClient  support for resource blacklisting
> ---------------------------------------------
>
>                 Key: YARN-771
>                 URL: https://issues.apache.org/jira/browse/YARN-771
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Bikas Saha
>            Assignee: Junping Du
>         Attachments: YARN-771-v1.0.patch, YARN-771-v2.patch
>
>
> After YARN-750 AMRMClient should support blacklisting via the new YARN API's

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message