[jira] [Commented] (YARN-1690) sending ATS events from Distributed shell

[Zhijie Shen (JIRA)]

    [ 
https://issues.apache.org/jira/browse/YARN-1690?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13938049#comment-13938049
 ] 

Zhijie Shen commented on YARN-1690:
-----------------------------------

Mayank, thanks for the new patch! It's almost good, except the following minor 
issues:

1. Call it DSEvent?
{code}
+  public static enum AppEvent {
{code}

2. Chang it to Timeline Client?
{code}
+  // ATS Client
{code}

3. Typo on "CLient"
{code}
+    // Creating the Application Timeline CLient
{code}

4.
bq. It has to be created, there is no previous config
config is the member field of ApplicationMaster
{code}
  // Configuration
  private Configuration conf;
{code}

5. Please merge the following duplicate exception handling as well
{code}
+        } catch (IOException e) {
+          LOG.error("Container start event could not be pulished for "
+              + containerStatus.getContainerId().toString());
+          LOG.error(e);
+        } catch (YarnException e) {
+          LOG.error("Container start event could not be pulished for "
+              + containerStatus.getContainerId().toString());
+          LOG.error(e);
+        }
{code}
{code}
+      } catch (IOException e) {
+        LOG.error("Container start event coud not be pulished for "
+            + container.getId().toString());
+        LOG.error(e);
+      } catch (YarnException e) {
+        LOG.error("Container start event coud not be pulished for "
+            + container.getId().toString());
+        LOG.error(e);
+      }
{code}

6. Again, please do not mention "AHS" here
{code}
+   * @return ApplicationTimelineStore for the AHS.
{code}

7. Please change publishContainerStartEvent, publishContainerEndEvent, 
publishApplicationAttemptEvent to static, which don't need to be per instance.

8. Please apply for the following to all the added error logs.
{code}
        LOG.error("Container start event coud not be pulished for "
            + container.getId().toString());
        LOG.error(e);
{code}
can be simplified as
{code}
        LOG.error("Container start event coud not be pulished for "
            + container.getId().toString(), e);
{code}

9. Please don't limit the output to 1. According to the args for this DS job, 
it should be 1 DS_APP_ATTEMPT entities and 2 DS_CONTAINER entities, which has 2 
events each? And assert the number of returned entities/events?
{code}
+        .getEntities(ApplicationMaster.DSEntity.DS_APP_ATTEMPT.toString(), 1l,
+            null, null, null, null, null);
{code}
{code}
+        .getEntities(ApplicationMaster.DSEntity.DS_CONTAINER.toString(), 1l,
+            null, null, null, null, null);
{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, 
> YARN-1690-4.patch, YARN-1690-5.patch
>
>




--
This message was sent by Atlassian JIRA
(v6.2#6252)