hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sangjin Lee (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-4675) Reorganize TimeClientImpl into TimeClientV1Impl and TimeClientV2Impl
Date Fri, 20 Jan 2017 22:10:27 GMT

    [ 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.

- l.57: agree with [~gtCarrera9] that we want to make the constructor protected

- 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

- 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 {{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
- 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?

- we should remove {{TimelineClientConnectionRetry}} and {{TimelineJerseyRetryFilter}}

- l.88: we should remove {{connectionRetry}}
- l.168: we should drop the {{boolean v2}} argument

- i believe we need to touch {{TestJobHistoryEventHandler}} too (the {{JHEvenHandlerForTest}}
- l.332, 458: super nit: space before the curly brace

- 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

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

View raw message