[
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)
