hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wangda Tan (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (YARN-3955) Support for priority ACLs in CapacityScheduler
Date Thu, 05 Jan 2017 19:46:00 GMT

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

Wangda Tan edited comment on YARN-3955 at 1/5/17 7:45 PM:
----------------------------------------------------------

1)
bq. There is one issue here. If approvedPriorityACL comes are null, for checkAccess it means
false.
Ok gotcha, my bad, we cannot merge the two. 

2)
Got it, not related to your patch. The previous design of "acl-key" is bad, it will be hard
to find which code path uses it...
And in addition, I didn't see test case that parses raw priority acls (string) to List of
PriorityACLGroup. Could you point me if there's any test cases exist?

Few renaming suggestions: 
- PriorityACLConfiguration \-> AppPriorityACLConfigurationParser (I was trying to find
where's the parser code, and since we're adding queue priority YARN-5864, so it will be better
to add an App\- to distinguish that) 
- Similiarily, PriorityAclConfig -> AppPriorityACLOwnerType (or any better name?)
- PriorityACLGroup -> AppPriorityACLGroup
- Do you think is it better to rename acl_access_priority to acl_app_max_priority? 

3)
bq. This code will reset to cluster-max priority only if submitted priority is more than cluster
max. Since I used compareTo, it not looks very readable.
Yeah, since we're using Priority in different ways, sometimes lower is more important and
sometimes higher is more important. Could you use ">" to do the comparision?

bq. checkAndGetApplicationPriority: when an app's priority set to negative, I think we should
use 0 instead of max. Thoughts?
Negative value looks fine, since app can set lower priority if needed.

4)
bq. Could I add a clear model. It may be more easy. Thoughts? Updated patch as per this. 
Not quite sure what did you mean. From my understanding, existing logic read acls from configs
while refreshQueues, and what we need to do is to replace all ACLs instead of append to previous
acl list, correct? 

bq. One doubt here. Since priorityAcls could also be updated in reinitialize, we can’t make
it as final rt. refreshQueue’s call flow for eg.
Since the returned list can be modified by another thread, so the readLock cannot provide
enough protection. The better way might be readLock + copyList.

bq. But we are doing statestore update within scheduler. Hence we need to pass future to see
exception is thrown immediately. Hence we had to add this while doing move to queue.
Make sense.


was (Author: leftnoteasy):
1)
bq. There is one issue here. If approvedPriorityACL comes are null, for checkAccess it means
false.
Ok gotcha, my bad, we cannot merge the two. 

2)
Got it, not related to your patch. The previous design of "acl-key" is bad, it will be hard
to find which code path uses it...
And in addition, I didn't see test case that parses raw priority acls (string) to List of
PriorityACLGroup. Could you point me if there's any test cases exist?

Few renaming suggestions: 
- PriorityACLConfiguration -> AppPriorityACLConfigurationParser (I was trying to find where's
the parser code, and since we're adding queue priority YARN-5864, so it will be better to
add an App- to distinguish that) 
- Similiarily, PriorityAclConfig -> AppPriorityACLOwnerType (or any better name?)
- PriorityACLGroup -> AppPriorityACLGroup
- Do you think is it better to rename acl_access_priority to acl_app_max_priority? 

3)
bq. This code will reset to cluster-max priority only if submitted priority is more than cluster
max. Since I used compareTo, it not looks very readable.
Yeah, since we're using Priority in different ways, sometimes lower is more important and
sometimes higher is more important. Could you use ">" to do the comparision?

bq. checkAndGetApplicationPriority: when an app's priority set to negative, I think we should
use 0 instead of max. Thoughts?
Negative value looks fine, since app can set lower priority if needed.

4)
bq. Could I add a clear model. It may be more easy. Thoughts? Updated patch as per this. 
Not quite sure what did you mean. From my understanding, existing logic read acls from configs
while refreshQueues, and what we need to do is to replace all ACLs instead of append to previous
acl list, correct? 

bq. One doubt here. Since priorityAcls could also be updated in reinitialize, we can’t make
it as final rt. refreshQueue’s call flow for eg.
Since the returned list can be modified by another thread, so the readLock cannot provide
enough protection. The better way might be readLock + copyList.

bq. But we are doing statestore update within scheduler. Hence we need to pass future to see
exception is thrown immediately. Hence we had to add this while doing move to queue.
Make sense.

> Support for priority ACLs in CapacityScheduler
> ----------------------------------------------
>
>                 Key: YARN-3955
>                 URL: https://issues.apache.org/jira/browse/YARN-3955
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: capacityscheduler
>            Reporter: Sunil G
>            Assignee: Sunil G
>         Attachments: ApplicationPriority-ACL.pdf, ApplicationPriority-ACLs-v2.pdf, YARN-3955.0001.patch,
YARN-3955.0002.patch, YARN-3955.0003.patch, YARN-3955.0004.patch, YARN-3955.0005.patch, YARN-3955.0006.patch,
YARN-3955.0007.patch, YARN-3955.0008.patch, YARN-3955.v0.patch, YARN-3955.v1.patch, YARN-3955.wip1.patch
>
>
> Support will be added for User-level access permission to use different application-priorities.
This is to avoid situations where all users try running max priority in the cluster and thus
degrading the value of priorities.
> Access Control Lists can be set per priority level within each queue. Below is an example
configuration that can be added in capacity scheduler configuration
> file for each Queue level.
> yarn.scheduler.capacity.root.<queue_name>.<priority>.acl=user1,user2



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

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


Mime
View raw message