[
https://issues.apache.org/jira/browse/YARN-7682?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16309205#comment-16309205
]
Konstantinos Karanasos edited comment on YARN-7682 at 1/3/18 6:33 AM:
----------------------------------------------------------------------
Thanks for the patch, [~pgaref]. Two main comments:
* I think that the functions you push in the getNodeCardinalityByOp should be
reversed. That is, I think when you have multiple tags, you should look at the
highest cardinality one in the node/rack, and your min cardinality of the
constraint should be below that. Same for the max cardinality of the constraint
(should be above the min of the actual cardinalities of the given tags). So
essentially you would define your minScopeCardinality using the Long::max, and
your maxScopeCardinality using the Long::min. Then everything goes as is. You
can also make a check that your minScopeCardinality <= maxScopeCardinality,
just to be on the safe side. Does it make sense?
* Do we need the line right after the comment “// Make sure Anti-affinity
satisfies hard upper limit”?
Nits:
* Let’s fill out the javadoc comments in canSatisfyConstraints
* In canSatisfySingleConstraintExpression the javadoc is a bit ambiguous. "The
node or rack should satisfy the constraints that are enabled by the given
allocation tags". Also let’s add javadoc comments for its parameters (I know it
is private, but it helps).
* In both methods you mention Node in the javadoc but you actually support RACK
too.
If the above are fixed, +1 from me.
was (Author: kkaranasos):
Thanks for the patch, [~pgaref]. Two main comments:
* I think that the functions you push in the getNodeCardinalityByOp should be
reversed. That is, I think when you have multiple tags, you should look at the
highest cardinality one in the node/rack, and your min cardinality of the
constraint should be below that. Same for the max cardinality of the constraint
(should be above the min of the actual cardinalities of the given tags). So
essentially you would define your minScopeCardinality using the Long::max, and
your maxScopeCardinality using the Long::min. Then everything goes as is. You
can also make a check that your minScopeCardinality <= maxScopeCardinality,
just to be on the safe side. Does it make sense?
* Do we need the line right after the comment “// Make sure Anti-affinity
satisfies hard upper limit”?
Nits:
* Let’s fill out the javadoc comments in canSatisfyConstraints
* In canSatisfySingleConstraintExpression the javadoc is a bit ambiguous. "The
node or rack should satisfy the constraints that are enabled by the given
allocation tags". Also let’s add javadoc comments for parameters its parameters
(I know it is private, but it helps).
* In both methods you mention Node in the javadoc but you actually support RACK
too.
If the above are fixed, +1 from me.
> Expose canAssign method in the PlacementConstraintManager
> ---------------------------------------------------------
>
> Key: YARN-7682
> URL: https://issues.apache.org/jira/browse/YARN-7682
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Arun Suresh
> Assignee: Panagiotis Garefalakis
> Attachments: YARN-7682-YARN-6592.001.patch,
> YARN-7682-YARN-6592.002.patch, YARN-7682-YARN-6592.003.patch,
> YARN-7682.wip.patch
>
>
> As per discussion in YARN-7613. Lets expose {{canAssign}} method in the
> PlacementConstraintManager that takes a sourceTags, applicationId,
> SchedulerNode and AllocationTagsManager and returns true if constraints are
> not violated by placing the container on the node.
> I prefer not passing in the SchedulingRequest, since it can have > 1
> numAllocations. We want this api to be called for single allocations.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
