[ https://issues.apache.org/jira/browse/YARN-3955?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15652791#comment-15652791 ]
Jian He commented on YARN-3955: ------------------------------- thanks Sunil, some comments for the patch: - PriorityACLConfiguration#getPrioirityAcl typo - I think we may not need a public API record PriorityACL as it's not supposed to be used by external application. May use AccessType#SUBMIT_APP_PRIORITY directly - AccessType#SUBMIT_APP_PRIORITY, maybe call it ACCESS_PRIORITY ? - make the log message clear that "user does not have permission to change priority" ? {code} RMAuditLogger.logFailure(callerUGI.getShortUserName(), AuditConstants.UPDATE_APP_PRIORITY, "User doesn't have permissions to " + ApplicationAccessType.MODIFY_APP.toString(), "ClientRMService", AuditConstants.UNAUTHORIZED_USER, applicationId); throw RPCUtil.getRemoteException(new AccessControlException("User " + callerUGI.getShortUserName() + " cannot perform operation " + ApplicationAccessType.MODIFY_APP.name() + " on " + applicationId)); {code} - I think readLock is not needed, the field itself is not changing. {code} try { readLock.lock(); return priorityAcls; } finally { readLock.unlock() {code} - could you add sample configurations in capacity-scheduler.xml, and add comments about what the syntax is for the config ? - the priority comes from submissionContext can be an arbitrary value, it's not guarated that "csqueue.getPriorityPrivilegedEntity(priority)" will always return the object? {code} Priority priority = submissionContext.getPriority(); if (null != csqueue) { if ((!authorizer.checkPermission( new AccessRequest(csqueue.getPrivilegedEntity(), userUgi, SchedulerUtils.toAccessType(QueueACL.SUBMIT_APPLICATIONS), applicationId.toString(), appName, Server.getRemoteAddress(), null)) && !authorizer.checkPermission( new AccessRequest(csqueue.getPrivilegedEntity(), userUgi, SchedulerUtils.toAccessType(QueueACL.ADMINISTER_QUEUE), applicationId.toString(), appName, Server.getRemoteAddress(), null))) || !authorizer.checkPermission(new AccessRequest( csqueue.getPriorityPrivilegedEntity(priority), userUgi, {code} - I feel, adding every possible priority at every integer for a queue into the ConfiguredYarnAuthorizer is cumbersome. One solution in my mind is to create a new QueueEntity which overridds PrivilegedEntity, and we add a max-priority field in the new object. On checkPermission, we check whether the accessType is accessPriority, if it is, check if the priority is less than the max-priority. AccessRequest also needs to add a new priority field. > 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 > Affects Versions: 2.7.1 > Reporter: Sunil G > Assignee: Sunil G > Attachments: ApplicationPriority-ACL.pdf, > ApplicationPriority-ACLs-v2.pdf, 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-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org