Sangjin Lee commented on YARN-3049:

Sorry [~zjshen] it took me a while to get to this. The patch looks pretty good 
actually. I have one high level point I'd like to discuss with you, and several 
smaller comments.

I see that you added a new boolean argument in 
{{TimelineCollector.putEntity()}}, {{TimelineCollector.putEntities()}}, and 
{{TimelineWriter.write()}} to indicate we're dealing with a new app (and thus 
writing to the app-to-flow table). I'm not sure whether that is really what we 
want to do. Can we not detect and leverage the fact that we're dealing with an 
"application created" event and trigger those actions instead of having an 
explicit argument that gets passed down all the way from the clients? First, in 
this approach we would be completely relying on the client code to specify this 
correctly. Secondly, I would argue that the fact that we need to detect that 
we're introducing a new application and write to these tables is somewhat of an 
"implementation detail" of the HBase writer. For example, other writers may not 
even care about that and have no need for it. The fact that this detail leaks 
all the way to the callers is awkward at best.

My initial thinking of how to do this was inside {{HBaseTimelineWriterImpl}} on 
detecting the application created event to trigger this action. What do you 

- l.138: it might be better to use the type {{SortedSet}} or {{NavigableSet}} 
to make it explicit we want ordering

- l.93: What does it mean to indicate newApp for a set of entities? What if the 
set of entities contains bunch of different applications?

- See comments above; rather than relying on the boolean flag in the arguments, 
can we detect the case of the application created event and do it?

- l.67: nit: I think the word "from" is needed there. It's just that the space 
was missing between "result" and "from".

- l.33: nit: "both matches" -> "both match"

> [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, 
> YARN-3049-YARN-2928.3.patch
> Implement existing ATS queries with the new ATS reader design.

This message was sent by Atlassian JIRA

Reply via email to