[ https://issues.apache.org/jira/browse/YARN-7522?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16275178#comment-16275178 ]
Konstantinos Karanasos commented on YARN-7522: ---------------------------------------------- Thanks again for the patch, [~leftnoteasy]. Please find some comments below. * 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? * The current getCardinality() tries to somehow impose constraints and at the same time check cardinalities. When I do a getCardinality over a set of tags, I am not sure it will be useful to get the max/min/sum of them. 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 would add one more getCardinality method that gets a single string tag and no binary operator just for simplicity. * It might be useful to have an exists method for a tag. Like checking without the exact cardinality whether this tag exists. * In getCardinality, what is the else doing when the specified tags are null or empty? * In RMContainerImpl, the TODO for setting the allocation tags should be part of this JIRA, right? * We probably need to clean up the tags of all containers of an app when the app finishes too. Will the current codepath take care of this even in cases of app failures etc.? * We should define a namespace for the appID, so that we can easily retrieve it. Like {{appID:}} and then the actual ID. Nits: * 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. * You can just have a boolean instead of the {{numValues}} in the {{getCardinality}}. * Why do we need the ImmutableSet in the add/removeContainer? * 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. > 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.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