[
https://issues.apache.org/jira/browse/YARN-955?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13814622#comment-13814622
]
Zhijie Shen commented on YARN-955:
----------------------------------
1. Not necessary. The default value can be read from yarn-default.xml. The
problem is that you can not specify the prefix variables like that in the xml
file. This default URI will be a relative path based on the current directory.
{code}
+ public static final String DEFAULT_FS_HISTORY_STORE_URI = "tmp";
{code}
2. Maybe just call it AHS_ADDRESS
{code}
+ public static final String AHS_HISTORY_ADDRESS = AHS_PREFIX + "address";
{code}
3. The nested class is not necessary. ApplicationHistoryClientService can
implement ApplicationHistoryProtocol directly.
{code}
+ private class ApplicationHSClientProtocolHandler implements
+ ApplicationHistoryProtocol {
{code}
4. Not necessary wrap-up. Please place the simple statement directly in the
callers. Same for getApplications.
{code}
+ public List<ApplicationAttemptReport> getApplicationAttempts(
+ ApplicationId appId) throws IOException {
+ List<ApplicationAttemptReport> appAttemptReports = new
ArrayList<ApplicationAttemptReport>(
+ history.getApplicationAttempts(appId).values());
+ return appAttemptReports;
+ }
{code}
5. Personally, I think returning empty collections is fine to indicate no
results. Otherwise, the caller needs always to check not null first.
{code}
+ } else {
+ response.setApplicationList(null);
+ }
{code}
6. Why do you want two references pointing to the same object?
{code}
+ historyService = createApplicationHistory();
+ historyContext = (ApplicationHistoryContext) historyService;
{code}
7. In the original design, we said we're going to make AHS a service of RM,
though it should be independent enough. In this patch, I can see AHS is going
to be an completely independent process. So far, it should be OK, because AHS
needs nothing from RM. However, I'm expecting some more security work to do if
AHS is separate process, as AHS and RM will not share the common context, and
may be launched by different users. [~vinodkv], do you have any opinion about
service or process?
Anyway, if we decide to make AHS a process now, this patch should also include
the shell script to launch AHS.
> [YARN-321] Implementation of ApplicationHistoryProtocol
> -------------------------------------------------------
>
> Key: YARN-955
> URL: https://issues.apache.org/jira/browse/YARN-955
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Vinod Kumar Vavilapalli
> Assignee: Mayank Bansal
> Attachments: YARN-955-1.patch, YARN-955-2.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.1#6144)
