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

Jian He commented on YARN-7605:
-------------------------------

- we can't assume if result != 0, it is ApplicationNotFoundException ? and 
instead of "result.intValue() == 0", it can be "result == 0"
{code}
      if (result.intValue() == 0) {
        ServiceStatus serviceStatus = new ServiceStatus();
        serviceStatus.setDiagnostics("Successfully stopped service " +
            appName);
        return Response.status(Status.OK).entity(serviceStatus).build();
      } else {
        throw new ApplicationNotFoundException("Service " + appName +
            " is not found in YARN.");
      }
{code}
- what is this NullPointerException for ? we should fix the NPE instead ?
{code}
    } catch (NullPointerException | IOException e) {
      LOG.error("Fail to stop service:", e);
{code}
- similarly, I think it is inappropriate to assume the cause of all these 
exceptions is service not found
{code}
    } catch (InterruptedException | ApplicationNotFoundException |
        UndeclaredThrowableException e) {
      ServiceStatus serviceStatus = new ServiceStatus();
      serviceStatus.setDiagnostics("Service " + appName +
          " is not found in YARN.");
{code}
- it is fine to just return the result 
{code}
return Integer.valueOf(result);
{code}
- create a common method for these ?
{code}
      UserGroupInformation proxyUser = UserGroupInformation.getLoginUser();
      UserGroupInformation ugi = UserGroupInformation
          .createProxyUser(request.getRemoteUser(), proxyUser);
{code}
- Again, it is inappropriate to assume all exceptions are caused by "Permission 
denied" ? this will confuse the users. And why not just user LOG.warn("...", 
e), but use a seaprate log statement for the exception ?
{code}
    try {
      proxy.flexComponents(requestBuilder.build());
    } catch (YarnException | IOException e) {
      LOG.warn("Exception caught during flex operation.");
      LOG.warn(ExceptionUtils.getFullStackTrace(e));
      throw new YarnException("Permission denied to perform flex operation.");
    }
{code}
- why is it needed to add the InterruptedException to the method signature, and 
make callers catch this exception and then convert to YarnException ?
{code}
  private ApplicationId submitApp(Service app)
      throws IOException, YarnException, InterruptedException {
{code}
- unnecessary formatting change in createAMProxy, actionSave

> Implement doAs for Api Service REST API
> ---------------------------------------
>
>                 Key: YARN-7605
>                 URL: https://issues.apache.org/jira/browse/YARN-7605
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Eric Yang
>            Assignee: Eric Yang
>             Fix For: yarn-native-services
>
>         Attachments: YARN-7605.001.patch, YARN-7605.004.patch, 
> YARN-7605.005.patch, YARN-7605.006.patch, YARN-7605.007.patch, 
> YARN-7605.008.patch, YARN-7605.009.patch
>
>
> In YARN-7540, all client entry points for API service is centralized to use 
> REST API instead of having direct file system and resource manager rpc calls. 
>  This change helped to centralize yarn metadata to be owned by yarn user 
> instead of crawling through every user's home directory to find metadata.  
> The next step is to make sure "doAs" calls work properly for API Service.  
> The metadata is stored by YARN user, but the actual workload still need to be 
> performed as end users, hence API service must authenticate end user kerberos 
> credential, and perform doAs call when requesting containers via 
> ServiceClient.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

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

Reply via email to