[
https://issues.apache.org/jira/browse/YARN-3049?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14649658#comment-14649658
]
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
think?
(TimelineEntity.java)
- l.138: it might be better to use the type {{SortedSet}} or {{NavigableSet}}
to make it explicit we want ordering
(TimelineCollector.java)
- 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?
(HBaseTimelineWriterImpl.java)
- 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?
(ColumnPrefix.java)
- l.67: nit: I think the word "from" is needed there. It's just that the space
was missing between "result" and "from".
(TimelineReaderUtils.java)
- 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
(v6.3.4#6332)
