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

Wangda Tan commented on YARN-7522:
----------------------------------

Thanks [~kkaranasos] for your comments, addressed most of your comments, see 
below.

bq. Should we start the tags manager in all cases or should we have a parameter 
for enabling the whole thing for the first version, given it will add some load 
to the RM?
I would prefer to enable it by default, in performance's perspective, it will 
be called twice for every container, I think it should be acceptable. I tend to 
not introduce too many knobs which complex how people use this feature.

bq. The current getCardinality() tries to somehow impose constraints and at the 
same time check cardinalities ... I would rather have a method that for a set 
of tags returns a set of their cardinalities so that my scheduler has the 
flexibility to decide what to do with them.
I'm completely aware of merits of your proposal. However, returning set of 
cardinalities needs an additional copy of the data structure. I'd prefer to 
push this and do it if it is really needed.

bq. I would add one more getCardinality method that gets a single string tag 
and no binary operator just for simplicity.
Done.

bq. It might be useful to have an exists method for a tag. Like checking 
without the exact cardinality whether this tag exists.
Done.

bq. In getCardinality, what is the else doing when the specified tags are null 
or empty?
When tags are null/empty, all tags exist on the node will be considered, just 
like passing in a wildcard. 

bq. In RMContainerImpl, the TODO for setting the allocation tags should be part 
of this JIRA, right?
No, the TODO needs to be done when we finish allocating container from 
SchedulingRequest from scheduler and scheduler set tags to RMContainer. 

bq. We probably need to clean up the tags of all containers of an app when the 
app finishes too. Will the current code path take care of this even in cases of 
app failures etc.?
I think so. This should be enough to cover RMContainer related updates, 
however, for latest proposal from Arun (see below), we need an external module 
to handle the update separately. 

bq. We should define a namespace for the appID, so that we can easily retrieve 
it. Like appID: and then the actual ID.
Done.

bq. In the PlacementConstraint(s) classes, we call the tags "allocation tags", 
so I suggest to rename the corresponding classes in this JIRA to AllocationTags 
instead of PlacementTags too.
Done.

bq. You can just have a boolean instead of the numValues in the getCardinality.
Done.

bq. Why do we need the ImmutableSet in the add/removeContainer?
Done, added single tag method to avoid creating ImmutableSet.

bq. In the removeTagsFromNode(), let's add some warn messages in case the node 
was not found or the count is already 0. It will help to catch bugs.
Done. 

Thanks [~asuresh], 

bq. Essentially, we need to simply be able to add/remove a tag to a node to 
allow the scheduler/planning system to keep track of node to tag mappings 
during intermediate processing as well.
Since AllocationTagsManager is a separate module and accessible from RMContext, 
it should be capable to support the use case you mentioned. However, the 
planning system needs to cleanup tags from external.


> Add application tags manager implementation
> -------------------------------------------
>
>                 Key: YARN-7522
>                 URL: https://issues.apache.org/jira/browse/YARN-7522
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Wangda Tan
>            Assignee: Wangda Tan
>         Attachments: YARN-7522.YARN-6592.002.patch, 
> YARN-7522.YARN-6592.003.patch, YARN-7522.YARN-6592.004.patch, 
> YARN-7522.YARN-6592.wip-001.patch
>
>
> This is different from YARN-6596, YARN-6596 is targeted to add constraint 
> manager to store intra/inter application placement constraints. This JIRA is 
> targeted to support storing maps between container-tags/applications and 
> nodes. This will be required by affinity/anti-affinity implementation and 
> cardinality.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org

Reply via email to