[
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
