[
https://issues.apache.org/jira/browse/YARN-1389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13923569#comment-13923569
]
Zhijie Shen commented on YARN-1389:
-----------------------------------
Thanks for the update, Mayank! The patch is general fine. Here're some
additional comments
1. is it simpler to use "e instanceof XXXXNotFoundException"?
{code}
+ // Even if history-service is enabled, treat all exceptions still the
same
+ // except the following
+ if (e.getClass() != ApplicationNotFoundException.class
+ && e.getClass() != ApplicationAttemptNotFoundException.class) {
+ throw e;
+ }
{code}
2. getFinishedStatus() is not necessary. You can directly do when() on
getDiagnostics/getExitStatus/getState.
{code}
+ ContainerStatus cs = mock(ContainerStatus.class);
+ when(containerimpl.getFinishedStatus()).thenReturn(cs);
+ when(containerimpl.getFinishedStatus().getDiagnostics()).thenReturn("N/A");
+ when(containerimpl.getFinishedStatus().getExitStatus()).thenReturn(0);
+ when(containerimpl.getFinishedStatus().getState()).thenReturn(
+ ContainerState.COMPLETE);
{code}
3. There're a lot of code duplication in TestClientRMService. You can move the
common code into a private createClientRMService method, which is called by
your test methods.
4. Shouldn't we remove "throw new YarnException("History service is not
enabled.");" in YarnClientImpl?
5. Shouldn't we assert fail here, because the exception is not excepted?
Similar in other test cases.
{code}
+ } catch (ApplicationNotFoundException ex) {
+ Assert.assertEquals(ex.getMessage(),
+ "Application with id '" + request.getApplicationAttemptId()
+ + "' doesn't exist in RM.");
+ }
{code}
In addition to that, personally, I'm still object to throwing
AppAttempt/Container not found exception when getting empty appattempt and
container list. Let's assume history service is disabled. Then, getting empty
applications is allowed while getting empty appattempt/container list will
result in exception. Th inconsistent behavior is going to confuse users. In
particular, It is likely that a running application doesn't have any appattempt
(e.g. the app is before ACCEPTED and is the first attempt).
> ApplicationClientProtocol and ApplicationHistoryProtocol should expose
> analogous APIs
> -------------------------------------------------------------------------------------
>
> Key: YARN-1389
> URL: https://issues.apache.org/jira/browse/YARN-1389
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Mayank Bansal
> Assignee: Mayank Bansal
> Attachments: YARN-1389-1.patch, YARN-1389-2.patch, YARN-1389-3.patch,
> YARN-1389-4.patch, YARN-1389-5.patch
>
>
> As we plan to have the APIs in ApplicationHistoryProtocol to expose the
> reports of *finished* application attempts and containers, we should do the
> same for ApplicationClientProtocol, which will return the reports of
> *running* attempts and containers.
> Later on, we can improve YarnClient to direct the query of running instance
> to ApplicationClientProtocol, while that of finished instance to
> ApplicationHistoryProtocol, making it transparent to the users.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
