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

Zhijie Shen commented on YARN-1250:
-----------------------------------

Thanks for review, Junping! I uploaded a new patch.

bq. In ApplicationHistoryManagerOnTimelineStore.java, checkAccess(app) get 
called duplicated in getApplicationAttempt(), getContainer(), etc. as it is 
already checked in generateApplicationReport within getApplication(). We should 
remove unnecessary check.

bq. We shouldn't swallow YARNException here especially in case we are removing 
duplicate checking due to above comments. Just do some necessary log and throw 
it out.

Good catch! However, checkAccess in generateApplicationReport doesn't throw 
exception to the caller. The reason is that when user doesn't have the access 
to the app, we show partial info (excluding the attempt details), and when user 
doesn't have the access to the attempt/container, we throw exception. 
ClientRMService has the analog behavior, and we need to keep them consistent. I 
modified generateApplicationReport's logic to stop early when we just pull 
USER_AND_ACLS fields.

bq. However, I would prefer these fixes going to be a separated patch which 
make this patch more focus.

Agree. Let's spin off this change

bq. I strongly suggest we have proper handling in SystemMetricsPublisher (like: 
log a warning or throw exception) for appACLsUpdated() and other operations 
when publishSystemMetrics is false in SystemMetricsPublisher.

We have corresponding INFO level log message to show whether system metrics 
publisher is enabled or not. If putting entity is failed, we will log an error 
log. And if DEBUG is enabled, we're going to record done all the entities that 
are going to be put. Hence I think it should be enough for testing and 
debugging.

bq. In your test case above, if getApplicationAttempt(appAttemptId) doesn't 
throw any exception, it still can pass. We should check an exception get thrown 
here. The same comments also for other places in the same file.

Fixed the problem.

> Generic history service should support application-acls
> -------------------------------------------------------
>
>                 Key: YARN-1250
>                 URL: https://issues.apache.org/jira/browse/YARN-1250
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Vinod Kumar Vavilapalli
>            Assignee: Zhijie Shen
>         Attachments: GenericHistoryACLs.pdf, YARN-1250.1.patch, 
> YARN-1250.2.patch, YARN-1250.3.patch, YARN-1250.4.patch, YARN-1250.5.patch
>
>




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

Reply via email to