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

Junping Du commented on YARN-1250:
----------------------------------

Thanks [~zjshen] for the design doc and patches!
+1 on proposed solutions in design doc. Some review comments on the patch:

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.

In generateApplicationReport(), 
{code}
+    } catch (YarnException e) {
+      // YarnExcetpion is thrown because the user doesn't have access
+      app.appReport.setDiagnostics(null);
+      app.appReport.setCurrentApplicationAttemptId(null);
+    }
{code}
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.

{code}
     } catch (Exception e) {
-      String message = "Failed to read the applications.";
+      String message = "Failed to read the applications: " + e.getMessage();
       LOG.error(message, e);
       html.p()._(message)._();
{code}
The fix here (and many other places) make sense to present exception to html 
pages, but make log message sounds duplicated. May be the better way is to keep 
original string, but add exception info later, something like: 
html.p()._(message + e.getMessage())._(); However, I would prefer these fixes 
going to be a separated patch which make this patch more focus.

{code}
+  public void appACLsUpdated(RMApp app, String appViewACLs,
+      long updatedTime) {
+    if (publishSystemMetrics) {
+      dispatcher.getEventHandler().handle(
+          new ApplicationACLsUpdatedEvent(
+              app.getApplicationId(),
+              appViewACLs,
+              updatedTime));
+    }
+  }
{code}
Looks like we don't know if appACLsUpdated get successfully or not. We even 
don't have any log or warning if timeline service is not enabled. 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.

In TestApplicationHistoryManagerOnTimelineStore.java, 
{code}
if (callerUGI == null) {
+      appAttempt = historyManager.getApplicationAttempt(appAttemptId);
+    } else {
+      try {
+        appAttempt =
+            callerUGI.doAs(new 
PrivilegedExceptionAction<ApplicationAttemptReport> () {
+          @Override
+          public ApplicationAttemptReport run() throws Exception {
+            return historyManager.getApplicationAttempt(appAttemptId);
+          }
+        });
+      } catch (UndeclaredThrowableException e) {
+        if (callerUGI != null && callerUGI.getShortUserName().equals("user3")) 
{
+          if (e.getCause().getMessage().contains(
+              "does not have privilage to see this application")) {
+            // The exception is expected
+            return;
+          }
+        }
+        throw e;
+      }
+    }
{code}
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.

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




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

Reply via email to