You are viewing a plain text version of this content. The canonical link for it is here.
Posted to yarn-issues@hadoop.apache.org by "Weiwei Yang (JIRA)" <ji...@apache.org> on 2018/02/26 06:47:00 UTC
[jira] [Comment Edited] (YARN-7965) NodeAttributeManager add/get
API is not working properly
[ https://issues.apache.org/jira/browse/YARN-7965?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16376445#comment-16376445 ]
Weiwei Yang edited comment on YARN-7965 at 2/26/18 6:46 AM:
------------------------------------------------------------
Thanks [~naganarasimha_gr@apache.org], [~sunilg]. Please help to review v2 patch.
bq. Ln no 187: change name from node to rmAttribute
Done
bq. ln no 318: why did we remove locking over here ?
I don't think the read lock is needed. It was protecting the read access to a ConcurrentHashMap, and the map is thread safe. Adding an extra lock is redundant.
bq. I think its a simple check of prefix.isEmpty() should have been !prefix.isEmpty() in my earlier code. Can we just have it in that way, it seems to be concise ?
If user provide a non-empty prefix set, and this set contains only 1 prefix which is not a valid one, the old logic will return all attributes to the client, this is a bug. Hence refactor the logic and I don't think the change complicate the code.
bq. License and checkstyle issues
Fixed.
[~sunilg] regarding the logs, [~naganarasimha_gr@apache.org] mentioned that he is working on a patch with more test cases and improvements, I assume that will help. This patch is for unblock rest of my patches so it only has minimal changes.
Hope that makes sense.
Thanks
was (Author: cheersyang):
Thanks [~naganarasimha_gr@apache.org], [~sunilg]. Please help to review v2 patch.
bq. Ln no 187: change name from node to rmAttribute
Done
bq. ln no 318: why did we remove locking over here ?
I don't think the read lock is needed. It was protecting the read access to a ConcurrentHashMap, and the map is thread safe. Adding an extra lock is redundant.
bq. I think its a simple check of prefix.isEmpty() should have been !prefix.isEmpty() in my earlier code. Can we just have it in that way, it seems to be concise ?
If user provide a non-empty prefix set, and this set contains only 1 prefix which is not a valid one, the old logic will return all attributes to the client, this is a bug. Hence refactor the logic and I don't think it complicate the code.
bq. License and checkstyle issues
Fixed.
[~sunilg] regarding the logs, [~naganarasimha_gr@apache.org] mentioned that he is working on a patch with more test cases and improvements, I assume that will help. This patch is for unblock rest of my patches so it only has minimal changes.
Hope that makes sense.
Thanks
> NodeAttributeManager add/get API is not working properly
> --------------------------------------------------------
>
> Key: YARN-7965
> URL: https://issues.apache.org/jira/browse/YARN-7965
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: resourcemanager
> Reporter: Weiwei Yang
> Assignee: Weiwei Yang
> Priority: Major
> Attachments: YARN-7965-YARN-3409.001.patch, YARN-7965-YARN-3409.002.patch
>
>
> Fix following issues,
> # After add node attributes to the manager, could not retrieve newly added attributes
> # Get cluster attributes API should return empty set when given prefix has no match
> # When an attribute is removed from all nodes, the manager did not remove this mapping
> and add UT
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org