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

Li Lu commented on YARN-3047:
-----------------------------

Hi [~varun_saxena]! Thanks for the patch. Here are some of my comments, sorry 
for the late reply. 

# In TimelineReaderStore, {{DEFAULT_LIMIT}} is never used. Do we want to keep 
this value here, or allow subclasses to access it, or we set up another 
configuration for this?
# NameValuePair duplicates the same file in ahs, timeline/NameValuePair. Since 
this is something used by both versions, maybe we can unify the two files 
somewhere? Maybe we'd like to avoid duplicating these files especially their 
contents are mostly the same and their paths are quite confusing. 
# In the original design, there was a reader pool that holds a set of readers. 
Each reader queries the underlying storage layer (and in future, the active 
apps). I was expecting this reader pool to be in TimelineReaderManager or 
TimelineReaderServer, but I could not find the logic to add or dispatch 
readers. Am I missing something here?
# In TimelineReaderWebServices, 
{code}
    try {
      return new NameValuePair(strs[0].trim(),
          GenericObjectMapper.OBJECT_READER.readValue(strs[1].trim()));
    } catch (Exception e) {
      return new NameValuePair(strs[0].trim(), strs[1].trim());
    }
{code}
Why are we catching Exceptions, rather than the precise exception readValue may 
throw?
# In TimelineReaderWebServices
{code}
  public AboutInfo about(
      @Context HttpServletRequest req,
      @Context HttpServletResponse res) {
    init(res);
    return new AboutInfo("Timeline API");
  }
{code}
This about info seems to be confusing. It's exactly the same as the v1 timeline 
server, but on our endpoint we only have the reader APIs available. 
# Just want to double check that we do want to have the same end point for both 
timeline readers and collectors. (Seems fine with me, since the reader process 
runs on different machines as our collectors. )
# We're using Java 7 now so we can use switch statements for strings:
{code}
if (s.equals("EVENTS")) {
        fieldList.add(Field.EVENTS);
      } else if (s.equals("LASTEVENTONLY")) {
        fieldList.add(Field.LAST_EVENT_ONLY);
      } else if (s.equals("METRICS")) {
        fieldList.add(Field.METRICS);
      } else if (s.equals("INFO")) {
        fieldList.add(Field.INFO);
      } else if (s.equals("CONFIGS")) {
        fieldList.add(Field.CONFIGS);
      } else {
        throw new IllegalArgumentException("Requested nonexistent field " + s);
      }
{code}
For more information: 
http://docs.oracle.com/javase/7/docs/technotes/guides/language/strings-switch.html
# TimelineReaderServer, do we have a fixed order to start/stop service for 
super?
# TimelineReaderServer, notice that the following lines:
{code}
TimelineReaderServer timelineReaderServer = null;
        try {
      timelineReaderServer = new TimelineReaderServer();
{code}
the line for the try statement is started with a tab instead of spaces. 

Still, I would be very appreciate if you could upload a simple writeup for the 
reader design. Thanks for working on this! 



> [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
>            Reporter: Sangjin Lee
>            Assignee: Varun Saxena
>         Attachments: YARN-3047.001.patch, YARN-3047.02.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