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

Sangjin Lee commented on YARN-3047:
-----------------------------------

Thanks for the update [~varun_saxena]!

Here is a quick review of the latest patch (v.9).

(yarn.cmd)
- l.153: why is "queue" removed from the list of commands?

(yarn-default.xml)
- l.1431: nit: space before the period (".")

(FileSystemTimelineReaderImpl.java)
- let's add annotations (private and unstable)

(TimelineReaderManager.java)
- let's add annotations (private and unstable)
- l.35: we don't need to override serviceInit()

(TimelineReaderServer.java)
- package: minor comment, but can we shorten the name a bit to 
{{org.apache.hadoop.yarn.server.timelineservice.reader}}?
- l.55: can it be private? should it be?
- l.64: Just curious, why use addService() for the timeline reader manager and 
addIfService() for the timeline reader store? Why not addService() for both as 
they are both services?
- l.111: nit: let's use Map (interface) as the type rather than HashMap

As for the tests (and for the configuration), do we have an ability to bind to 
the "available" port so that we do not rely on fixed ports? I believe we did 
that for the writer service, and without that tests might fail, depending on 
what else may be using the fixed port at the time.


> [Data Serving] Set up ATS reader with basic request serving structure and 
> lifecycle
> -----------------------------------------------------------------------------------
>
>                 Key: YARN-3047
>                 URL: https://issues.apache.org/jira/browse/YARN-3047
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>    Affects Versions: YARN-2928
>            Reporter: Sangjin Lee
>            Assignee: Varun Saxena
>              Labels: BB2015-05-TBR
>         Attachments: Timeline_Reader(draft).pdf, 
> YARN-3047-YARN-2928.08.patch, YARN-3047-YARN-2928.09.patch, 
> YARN-3047.001.patch, YARN-3047.003.patch, YARN-3047.005.patch, 
> YARN-3047.006.patch, YARN-3047.007.patch, YARN-3047.02.patch, 
> YARN-3047.04.patch
>
>
> Per design in YARN-2938, set up the ATS reader as a service and implement the 
> basic structure as a service. It includes lifecycle management, request 
> serving, and so on.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to