[
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 big.
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 me.
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, YARN-4675-YARN-2928.v1.001.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
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
