[
https://issues.apache.org/jira/browse/YARN-967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13827975#comment-13827975
]
Zhijie Shen commented on YARN-967:
----------------------------------
Reviewed the latest patch. Here're some comments
1. Change yarn.cmd accordingly.
2. Not necessary, no log is written in AHSClientImpl.
{code}
+ private static final Log LOG = LogFactory.getLog(AHSClientImpl.class);
{code}
3. Where're the following configurations? Defined in other patch?
{code}
+ return conf.getSocketAddr(YarnConfiguration.AHS_ADDRESS,
+ YarnConfiguration.DEFAULT_AHS_ADDRESS,
+ YarnConfiguration.DEFAULT_AHS_PORT);
{code}
4. Should AHSClientImpl use YarnClient configurations?
{code}
+ statePollIntervalMillis = conf.getLong(
+ YarnConfiguration.YARN_CLIENT_APP_SUBMISSION_POLL_INTERVAL_MS,
+ YarnConfiguration.DEFAULT_YARN_CLIENT_APP_SUBMISSION_POLL_INTERVAL_MS);
{code}
5. Is the following condition correct?
{code}
+ } else if (args[0].compareToIgnoreCase(APPLICATION_ATTEMPT) == 0
+ || args[0].compareToIgnoreCase(CONTAINER) == 0) {
{code}
6. One important issue here is that the command change is incompatible. The
users' old shell scripts will be break given the change here. It's good to make
the command compatible. For example, by default, it's going to the info of the
application(s). Or at least, we need to document the new behavior of the
command. [~vinodkv], how do you say?
7. Rename it to appAttemptReportStr? Also the javadoc.
{code}
+ PrintWriter appReportStr = new PrintWriter(baos);
{code}
{code}
+ * Prints the application report for an application id.
{code}
8. Fix the above issue for printContainerReport as well.
9. Does AHS RPC protocol throw not found exception as well? If not, I think
it's good to do that to keep consistent. Maybe do the same for
getApplicationAttemptReport and getContainerReport
{code}
+ if (appReport == null) {
+ appReport = historyClient.getApplicationReport(ConverterUtils
+ .toApplicationId(applicationId));
+ }
{code}
10. Check getApplications as well. Make getApplicationAttempts and
getContainers behave similarly. This and the one above are the server-side
changes. Probably you'd like to coordinate your other patches.
11. For listApplications, if the users want the applications in
FINISHED/FAILED/KILLED states, why not going to historyClient as well?
12. AHSProxy is using a bunch of RM configurations instead of AHS ones. By the
way, it seems AHSProxy is almost the same as RMProxy. Is it possible to reuse
the code instead of duplicating it?
13. In YarnCLI, should we make getter for historyClient as well, like client?
14. The mock doesn't need to be defined in getXXXX and invoked every time
getXXXX is called. Define it once, it will behave the same in the following.
{code}
when(mockAppResponse.getApplicationList()).thenReturn(reports);
{code}
15. It's better to mock multiple attempts/containers to test getXXXXs.
16. The modified part of ApplicationCLI needs to be tested as well.
> [YARN-321] Command Line Interface(CLI) for Reading Application History
> Storage Data
> -----------------------------------------------------------------------------------
>
> Key: YARN-967
> URL: https://issues.apache.org/jira/browse/YARN-967
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Devaraj K
> Assignee: Mayank Bansal
> Attachments: YARN-967-1.patch, YARN-967-2.patch, YARN-967-3.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.1#6144)
