[
https://issues.apache.org/jira/browse/YARN-4675?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15832483#comment-15832483
]
Sangjin Lee commented on YARN-4675:
-----------------------------------
Thanks [~Naganarasimha]! I went over the latest patch, and have some comments.
Quite a few comments overlap with [~gtCarrera9]'s comments.
(TimelineClient.java)
- l.57: agree with [~gtCarrera9] that we want to make the constructor protected
(TimelineV2Client.java)
- I would move {{contextAppId}} to {{TimelineV2ClientImpl}}. It is used solely
internally by {{TimelineV2ClientImpl}} and it should be defined there.
- As a follow-up, we should drop the {{appId}} argument from the
{{TimelineV2Client}} constructor
- As a follow-up, we can drop the {{setContextAppId}} and {{getContextAppId}}
methods
- l.37: The blurb "Create a timeline client" sounds strange. We should remove
that sentence once it's moved.
- l.58: make this constructor protected too
- l.78,96: nit: let's use imports now that there is no ambiguity
(TimelineClientImpl.java)
- we should do a strong check of the timeline service version in
{{serviceInit()}} not to accept an invalid version
- l.89: this should be removed
- we should remove {{pollTimelineServiceAddress()}}
- we should remove {{initConnConfigurator()}}
- we should remove {{initSslConnConfigurator()}}
- we should remove {{serviceRetryInterval}}
- we should remove {{DEFAULT_TIMEOUT_CONN_CONFIGURATOR}}
- we should remove {{setTimeouts()}}
- we should remove {{DEFAULT_SOCKET_TIMEOUT}}
- {{TimelineClientConnectionRetry}} and {{TimelineJerseyRetryFilter}} are
duplicated with {{TimelineServiceHelper}}; we should determine where we need
them and remove duplication; I would remove them from {{TimelineServiceHelper}}
unless we think we will use them later for v.2
- l.562: we should drop the {{boolean v2}} argument
- we don't need a public {{setTimelineServiceAddress()}} method in v.1.x; I
would simply inline it
- 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?
(TimelineServiceHelper.java)
- we should remove {{TimelineClientConnectionRetry}} and
{{TimelineJerseyRetryFilter}}
(TimelineV2ClientImpl.java)
- l.88: we should remove {{connectionRetry}}
- l.168: we should drop the {{boolean v2}} argument
(JobHistoryEventHandler.java)
- i believe we need to touch {{TestJobHistoryEventHandler}} too (the
{{JHEvenHandlerForTest}} class)
- l.332, 458: super nit: space before the curly brace
(TestJobHistoryEventHandler.java)
- is this all about testing timeline service v.1? note that
{{mockAppContext()}} now returns a v.1 client only
> 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-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: [email protected]
For additional commands, e-mail: [email protected]
