[
https://issues.apache.org/jira/browse/YARN-955?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13794196#comment-13794196
]
Zhijie Shen commented on YARN-955:
----------------------------------
[~mayank_bansal], thanks for the work on the implementation of
ApplicationHistoryProtocol. Here're some comments:
1. Is there any special reason to rename ASHService to
ApplicationHistoryClientService?
2. Inner ApplicationHSClientProtocolHandler is not necessary.
ApplicationHistoryClientService can directly implement
ApplicationHistoryProtocol, which is what ASHService did before.
{code}
+ this.protocolHandler = new ApplicationHSClientProtocolHandler();
{code}
3. Incorrect log bellow:
{code}
+ LOG.info("Instantiated MRClientService at " + this.bindAddress);
{code}
4. We should use the newInstance method from the record class for
GetApplicationAttemptReportResponse and all the other records.
{code}
+ GetApplicationAttemptReportResponse response =
+ recordFactory
+ .newRecordInstance(GetApplicationAttemptReportResponse.class);
{code}
5. Some methods missed \@Override, for example
{code}
+ public ApplicationAttemptReport getApplicationAttempt(
{code}
6. The two methods bellow is not implemented, but we can do it separately,
because we need to implement a DelegationTokenSecretManager first.
{code}
+ @Override
+ public GetDelegationTokenResponse getDelegationToken(
+ GetDelegationTokenRequest request) throws YarnException, IOException {
+ // TODO Auto-generated method stub
+ return null;
+ }
+
+ @Override
+ public RenewDelegationTokenResponse renewDelegationToken(
+ RenewDelegationTokenRequest request) throws YarnException, IOException
{
+ // TODO Auto-generated method stub
+ return null;
+ }
{code}
7. Did you miss ApplicationHistoryContext in the patch or is it included in the
patch of other Jira?
{code}
+ historyContext = (ApplicationHistoryContext) historyService;
{code}
8. Why the method bellow has the default access control?
{code}
+ static ApplicationHistoryServer launchAppHistoryServer(String[] args) {
{code}
9. In RM and NM, we usually add a protected createXXXX() method for a sub
service, such that we can override it, and change to another implementation. It
is convenient when we want to mock some part of AHS when drafting the test
cases.
{code}
+ ahsClientService = new ApplicationHistoryClientService(historyContext);
{code}
10. Shall we have the test cases for the ApplicationHistoryProtocol
implementation?
> [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
>
>
--
This message was sent by Atlassian JIRA
(v6.1#6144)
