[
https://issues.apache.org/jira/browse/YARN-1690?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13904975#comment-13904975
]
Zhijie Shen commented on YARN-1690:
-----------------------------------
Thanks for the patch. Here're some comments:
1. Catch Exception and merge the duplicate handling.
{code}
+ } catch (IOException e) {
+ LOG.error("App Attempt start event coud not be pulished for "
+ + appAttemptID.toString());
+ LOG.error(e);
+ } catch (YarnException e) {
+ LOG.error("App Attempt start event coud not be pulished for "
+ + appAttemptID.toString());
+ LOG.error(e);
+ }
{code}
2. Call it timeline client, and similar for the following code.
{code}
+ // ATS Client
+ private static TimelineClient atsClient;
{code}
3. Is the following the related change
{code}
- System.exit(1);
+ LogManager.shutdown();
+ ExitUtil.terminate(1, t);
{code}
4. Don't create a new config, but use the existing one.
{code}
+ Configuration config = new YarnConfiguration();
{code}
5. Call it DS_CONTAINER? Do not confuse it with the generic information.
{code}
+ entity.setEntityType("ContainerId");
{code}
6. Entity type is different from event type. Call it DS_APPLICATION_ATTEMPT?
{code}
+ entity.setEntityType(appEvent.toString());
{code}
7. Event type is not set
{code}
+ ATSEvent event = new ATSEvent();
+ event.setTimestamp(System.currentTimeMillis());
+ entity.addEvent(event);
{code}
8. Correct "STatus"
{code}
+ event.addEventInfo("Exit STatus", container.getExitStatus());
{code}
9. Can you add user as the primary filter?
10. In general, it doesn't make sense to record the information that the
generic history service has already captured, such as the other info for
container. It's per-framework data, such that it's better to record some DS
specific information.
11. Need more assertion. For example, test both container and attempt entities.
{code}
+ Assert.assertNotNull(entities);
+ Assert.assertEquals(entities.getEntities().get(0).getEntityType()
+ .toString(), "ContainerId");
{code}
12. Mark it \@Private as well
{code}
+ public ApplicationTimelineStore getTimelineStore() {
{code}
13. Correct comment? It seems you choose to set default AHS address, and don't
understand why it is related to YARN_MINICLUSTER_FIXED_PORTS.
{code}
+ if (!conf.getBoolean(YarnConfiguration.YARN_MINICLUSTER_FIXED_PORTS,
+ YarnConfiguration.DEFAULT_YARN_MINICLUSTER_FIXED_PORTS)) {
+ // pick free random ports.
+ conf.set(YarnConfiguration.AHS_ADDRESS,
+ YarnConfiguration.DEFAULT_AHS_ADDRESS);
+ conf.set(YarnConfiguration.AHS_WEBAPP_ADDRESS,
+ YarnConfiguration.DEFAULT_AHS_WEBAPP_ADDRESS);
+ }
{code}
> sending ATS events from Distributed shell
> ------------------------------------------
>
> Key: YARN-1690
> URL: https://issues.apache.org/jira/browse/YARN-1690
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Mayank Bansal
> Assignee: Mayank Bansal
> Attachments: YARN-1690-1.patch, YARN-1690-2.patch, YARN-1690-3.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
