[
https://issues.apache.org/jira/browse/YARN-8002?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16399443#comment-16399443
]
Wangda Tan commented on YARN-8002:
----------------------------------
Thanks [~cheersyang], the latest patch looks much better now.
Some additional minor comments:
1) Changes in SingleConstraintAppPlacementAllocator:
- Before the patch only SELF supported, after this patch several ALLOCATION_TAG
supported. I'm not sure if we should do that since we haven't validated if
inter-app target works for SingleConstraintAppPlacementAllocator.
2) TargetApplications:
- Removed unused methods.
3) aggregateAllocationTagsByApps/aggregateAllocationTagsByRack
- The two methods can be merged.
- appIds != null check is redundant (since
{{AllocationTagNamespace#getNamespaceScope}} return non-null always).
4) getNamespaceScope:
- There're several "scope" related methods, I suggest to rename them to
get/setApplicationIds. The name of "scope" is confusing to me.
- Following methods are used by test only, suggest to remove them:
{code}
/**
* @return true if the namespace is effective in all applications
* in this cluster. Specifically the namespace prefix should be
* "all".
*/
public boolean isGlobal() {
return AllocationTagNamespaceType.ALL.equals(getNamespaceType());
}
/**
* @return true if the namespace is effective within a single application
* by its application ID, the namespace prefix should be "app-id";
* false otherwise.
*/
public boolean isSingleInterApp() {
return AllocationTagNamespaceType.APP_ID.equals(getNamespaceType());
}
/**
* @return true if the namespace is effective to the application itself,
* the namespace prefix should be "self"; false otherwise.
*/
public boolean isIntraApp() {
return AllocationTagNamespaceType.SELF.equals(getNamespaceType());
}
/**
* @return true if the namespace is effective to all applications except
* itself, the namespace prefix should be "not-self"; false otherwise.
*/
public boolean isNotSelf() {
return AllocationTagNamespaceType.NOT_SELF.equals(getNamespaceType());
}
{code}
And actually I suggest to remove all the is... methods since we can check
{{AllocationTagNamespaceType.XYZ.equals(getNamespaceType)}}. This is optional
to me, depends on you.
- Several checks are check unsupported namespaces:
{code}
// TODO Complete remove this check once we support app-label.
if (namespace.isAppLabel()) {
throw new InvalidAllocationTagsQueryException(
namespace.toString() + " is not supported yet!");
}
{code}
I suggest to add a method like {{throwExceptionIfNamespaceTypeNotSupported}} so
we don't need to change all the places in the future.
> Support NOT_SELF and ALL namespace types for allocation tag
> -----------------------------------------------------------
>
> Key: YARN-8002
> URL: https://issues.apache.org/jira/browse/YARN-8002
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: resourcemanager
> Reporter: Weiwei Yang
> Assignee: Weiwei Yang
> Priority: Major
> Attachments: YARN-8002.001.patch, YARN-8002.002.patch,
> YARN-8002.003.patch
>
>
> This is a continua task after YARN-7972, YARN-7972 adds support to specify
> tags with namespace SELF and APP_ID, like following
> * self/<tag>
> * app-id/<appid>/<tag>
> this task is to track the work to support 2 of remaining namespace types
> *NOT_SELF* & *ALL* (we'll support app-label later),
> * not-self/<tag>
> * all/<tag>
> this will require a bit refactoring in {{AllocationTagsManager}} as it needs
> to do some proper aggregation on tags for multiple apps.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
