hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Naganarasimha G R (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (YARN-4675) Reorganize TimeClientImpl into TimeClientV1Impl and TimeClientV2Impl
Date Mon, 23 Jan 2017 09:20:27 GMT

     [ https://issues.apache.org/jira/browse/YARN-4675?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel

Naganarasimha G R updated YARN-4675:
    Attachment: YARN-4675.v2.004.patch

Thanks [~gtCarrera9] and [~sjlee0], for detailed reviews, and sorry had missed to place the
latest TimelineClientImpl due to which some of the duplication comments came up. Hope i have
addressed all the comments in the latest patch, please ping  me if have missed any. Further

bq. DistributedShell: Can we unify all {{if}}s for publishing timeline event? We may want
to have centralized methods to dispatch timeline client calls to v1 and v2.
Not sure about this as the methods and arguments which are passed is totally different for
each event published. Also IMO cleaning up of distributed shell can be taken up as separate
jira if required as it seems to be over the scope of this jira and current patch already seems

bq. TimelineClient : Let's refer to TimelineV2Client in the Java doc for v2 use cases?
Did not get this review comment. IIUC you wanted to have @see TimelineV2Client in TimelineClient

bq. Shall we close the constructor to protected since we've experienced some unexpected calls
to it in v1? 
To do this we need to move impls of V1 and v2 in the same package as that of TimelineClient,
if all are ok then fine with me too(will do it in next patch)

bq. Do we allow reusing timeline v2 clients across multiple applications?
IMO it should be immutable like you mentioned but not sure why earlier it was done in that
way, anyway have removed the setter

bq. There are two {{TimelineServiceHelper}}s in our codebase? One is really trivial. Shall
we merge them or eliminate one of them?
Agree one is trivial, but the one introduced by me is more like used only by the ats v1/v2
impls and the other one by api records, hence modified the name of the class introduced by
bq. I would remove them from TimelineServiceHelper unless we think we will use them later
for v.2
Initially i too thought the same but later thought that its not particular to V1 client as
such, hence have moved it to helper class itself and if required we can make use of it in
v2 in future.

bq. TimelineClientImpl : in putEntities(), we can remove the check if we have it in serviceInit();
were we always checking for == 1.5 by the way? So we don't support 1.0 any more?
This API was introduced in 1.5 in specific, hence i dont think we can have it in {{serviceInit}}

bq. TestJobHistoryEventHandler: is this all about testing timeline service v.1?
Seems like {{TestJobHistoryEventHandler}} is currently only for v.1, if required we need to
raise a jira to handle for v2 too. Earlier test were capturing the basic scenario in TestMRTimelineEventHandling.

> Reorganize TimeClientImpl into TimeClientV1Impl and TimeClientV2Impl
> --------------------------------------------------------------------
>                 Key: YARN-4675
>                 URL: https://issues.apache.org/jira/browse/YARN-4675
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Naganarasimha G R
>            Assignee: Naganarasimha G R
>              Labels: YARN-5355, yarn-5355-merge-blocker
>         Attachments: YARN-4675.v2.002.patch, YARN-4675.v2.003.patch, YARN-4675.v2.004.patch,
> We need to reorganize TimeClientImpl into TimeClientV1Impl ,  TimeClientV2Impl and if
required a base class, so that its clear which part of the code belongs to which version and
thus better maintainable.

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org

View raw message