[
https://issues.apache.org/jira/browse/YARN-6858?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16356522#comment-16356522
]
Weiwei Yang commented on YARN-6858:
-----------------------------------
Hi [~Naganarasimha]
Thanks for the patch. I am focusing on the manager API and implementation in
this patch, please see my comments below,
*AttributeNodeLabelsManager*
# Can we rename this to just {{NodeAttributesManager}}?
{{AttributeNodeLabelsManager}} is a bit odd name to me.
# Missing java doc for this class
# Missing apache license
# public abstract Map<String, Set<String>> getAttributesToNodes(); --- key can
be {{NodeAttribute}} instead of String
*AttributeNodeLabelsManagerImpl*
# Can we rename this to {{NodeAttributesManagerImpl}} ?
# Missing apache license
# line 50: can we just use {{AsyncDispatcher}} to avoid class cast?
# line 80: It uses {{yarn.node-labels.enabled}} flag to determine if attributes
manager is enabled or not. This means an user can only enable or disable labels
and attributes together. Can we use a separate configuration property?
# Method names: removeNodeFromLabels, addNodeToLabels, replaceNodeForLabels ..
can we replace {{Labels}} to {{Attributes}}?
# line 204: {{validateAndGetNodeToAttribMap}} to
{{validateAndGetNodeToAttributesMap}}, a small typo. And this method seems to
handle validation for both add/remove attributes, which makes the code very
hard to read. I think we can wrap up some common logic in an utility class and
have separate checks for add/update/remove. Second reason to do so is that in
future we need to add proper ACLs for each operation.
# Some concern over {{internalUpdateLabelsOnNodes}}. When user adds a
node-attribute, attribute-manager first updates the mapping in memory, then do
an update on a configured store by handling a {{NodeAttributesStoreEvent}}.
What if memory update succeed but store update fails? This seems would bring
inconsistent state.
# line 409 class Host. I understand we need to maintain states when node state
changes. But why we need this inner class to handle this? Why not we directly
use {{RMNode}} abstraction and let it carry a set of {{NodeAttribute}}s?
Thanks
> Attribute Manager to store and provide the attributes in RM
> -----------------------------------------------------------
>
> Key: YARN-6858
> URL: https://issues.apache.org/jira/browse/YARN-6858
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: api, capacityscheduler, client
> Reporter: Naganarasimha G R
> Assignee: Naganarasimha G R
> Priority: Major
> Attachments: YARN-6858-YARN-3409.001.patch,
> YARN-6858-YARN-3409.002.patch
>
>
> Similar to CommonNodeLabelsManager we need to have a centralized manager for
> Node Attributes too.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
