[ 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