Varun Saxena commented on YARN-4075:

[~gtCarrera9], thanks for the review.

bq. Maybe we'd like to return the "default" cluster, or the cluster the reader 
runs on (or a reader farm associates to), if the given clusterId is empty?
Infact in initial patches in YARN-3814 I was taking cluster ID from config if 
it was not supplied by user i.e. it was an optional query parameter. But Zhijie 
was of the opinion that this handling should be done at TimelineClient side and 
that is what seemed to be the consensus at that time. Hence I removed it. If 
the consensus now seems to be centering around handling it at server side, we 
can do that. I am fine either ways.

bq. I just noticed that we're returning Set<TimelineEntity> rather than 
TimelineEntities in timeline reader.
Ok. Again I had initially kept the API as returning TimelineEntities in 3051 
but opinion differed then. I would infact prefer using TimelineEntities. For 
ordering should we change the set inside TimelineEntities to TreeSet with 
comparator based on created time ? Ordering might be useful at the client side.

bq. In TestTimelineReaderWebServicesFlowRun#testGetFlowRun, why do we compare 
equality through toString and comparing two strings
For the sake of simplicity. The toString outputs values as well. Anyways I can 
write a static function in the test class as well to do the comparison if 
toString approach seems confusing. Seems to be the case. Will change it.

bq. Any special reasons to refactor TestHBaseTimelineStorage
Due to visibility of TimelineSchemaCreator#createAllTables. Saw no real need to 
make it public. WebServices related test class shouldn't really need to access 
it directly. As I said in one of the comments above, I plan to combine to test 
webservices classes to use HBase. For that I will have a Test class for hbase 
reader implementation which will create the tables and load the data (in some 
beforeclass method). Webservices class will merely call that. Same arrangement 
as the one which exists for TestTimelineReaderWebServices and 
TestFileSystemTimelineReaderImpl. Then I wont need to call createAllTables from 
this test class. Will do that refactoring in some reader related JIRA. At the 
time, getting in this JIRA was more important than this refactoring, which 
anyways is just for tests. 

> [reader REST API] implement support for querying for flows and flow runs
> ------------------------------------------------------------------------
>                 Key: YARN-4075
>                 URL: https://issues.apache.org/jira/browse/YARN-4075
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>    Affects Versions: YARN-2928
>            Reporter: Sangjin Lee
>            Assignee: Varun Saxena
>         Attachments: YARN-4075-YARN-2928.POC.1.patch, 
> YARN-4075-YARN-2928.POC.2.patch
> We need to be able to query for flows and flow runs via REST.

This message was sent by Atlassian JIRA

Reply via email to