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

Li Lu commented on YARN-3049:
-----------------------------

Hi [~zjshen], some of my comments:

- The addition on {{newApp}} is to indicate if we need if we need to update the 
app2flow index table. This change is an interface change and it's slightly more 
than I thought. However, I still incline to proceed the changes in this JIRA so 
that we can speed up consolidating our POC patches. 

- FileSystemTimelineReaderImpl, in {{fillFields}}, maybe we can use 
EnumSet.allOf() to generate the universe of fields so that we can reuse the 
logic of the following for loop for Field.ALL? 

- Reader interface: use TimelineCollectorContext to package reader arguments?

- HBaseTimelineReaderImpl:
l.160 (all line numbers are after patch)
{code}
byte[] row = result.getRow();
{code}
unused? 

l.213 name of private method {{getEntity}}: I think we may want to distinguish 
that with the external {{getEntity}} API. How about parseEntity or 
getEntitiFromResult? 

We're now performing filters by ourselves in memory. I'm wondering if it will 
be more efficient to translate some of our filter specifications into HBase 
filters? 

l.113, 136, 142: I'm a little bit worry about the {{0L}}s. Shall we have 
something like DEFAULT_TIME to make the argument list more readable? 

I assume the problem raised in l.369 ("if the event come with no info, it will 
be missed") will be addressed after YARN-3984? 

- HBaseTimelineWriterImpl:
l.121-122: The log information is unclear about the write happened onto the 
App2Flow table? Also, we may want to keep this message in debug level?

- TimelineSchemaCreator:
Why we are not adding {{a2f}} as an option, similar to what we did in l.94-102 
for {{e}} and {{m}}?

- App2FlowColumn:
l.51, {{private}} appears to be redundant in enums. Similarly in l.42 or 
App2FlowColumnFamily. 

nits: 
- Name of App2FlowTable, AppToFlowTable? Saving one character every time is not 
quite helpful...

- l. 248, 263, 336: I'm confused by the name readConnections...

- Add a specific test in TestHBaseTimelineWriterImpl for App2FlowTable? 

> [Storage Implementation] Implement storage reader interface to fetch raw data 
> from HBase backend
> ------------------------------------------------------------------------------------------------
>
>                 Key: YARN-3049
>                 URL: https://issues.apache.org/jira/browse/YARN-3049
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Sangjin Lee
>            Assignee: Zhijie Shen
>         Attachments: YARN-3049-WIP.1.patch, YARN-3049-WIP.2.patch, 
> YARN-3049-WIP.3.patch, YARN-3049-YARN-2928.2.patch
>
>
> Implement existing ATS queries with the new ATS reader design.



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

Reply via email to