Mailing-List: contact yarn-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Date: Mon, 23 Jan 2017 15:34:26 +0000 (UTC)
From: "Varun Saxena (JIRA)" <jira@apache.org>
To: yarn-issues@hadoop.apache.org
Message-ID: <JIRA.12937089.1454693500000.80846.1485185666842@Atlassian.JIRA>
In-Reply-To: <JIRA.12937089.1454693500000@Atlassian.JIRA>
References: <JIRA.12937089.1454693500000@Atlassian.JIRA> <JIRA.12937089.1454693500302@jira-lw-us.apache.org>
Subject: [jira] [Comment Edited] (YARN-4675) Reorganize TimeClientImpl into
 TimeClientV1Impl and TimeClientV2Impl
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Mon, 23 Jan 2017 15:34:33 -0000


    [ https://issues.apache.org/jira/browse/YARN-4675?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15834758#comment-15834758 ] 

Varun Saxena edited comment on YARN-4675 at 1/23/17 3:34 PM:
-------------------------------------------------------------

Thanks [~naganarasimha_gr@apache.org] for the latest patch. Few comments.

# There is a bit of connection related duplicate code in TimelineClientImpl and TimelineClientV2Impl#serviceInit. I think we can turn TimelineClientHelper into a service (or a standalone class whose instance can be created) which can be initialized and stopped (to do the cleanup). We can encapsulate all Jersey client related stuff and connection retry logic in this class. If timeline client implementations want to access something from this class, we can provide relevant getters. I think this would be more cleaner.
# There are a lot of variables in both TimelineClientImpl and TimelineClientV2Impl which need not be class member variables as they are not referred to only during initialization.
# I agree in principle with the comment above i.e. to reduce the visibility of TimelineClient*Impl constructor. I think we can move the classes to same package as TimelineClient and TimelineV2Client. As TimelineClientImpl is annotated as Private, this should be fine, backward compatibility wise as well.
# In MRAppMaster#RunningAppContext, the comment over getTimelineClient method isn't suitable.
# ApplicationMaster Line 302, timelineV2Client need not be made visible for testing.
# We will reuse TimelineClientConnectionRetry for V2 as well so I think its fine to move it.
# TimelineV2Client, Line 35, we should say @see TimelineV2Client instead of TimelineClient
# In TimelineClient we had the below javadoc which has been removed in the patch. It was wrongly placed over contextAppId before. But the javadoc need not be removed. It should be placed over method createTimelineClient.
{code}
49	  /**
50	   * Create a timeline client. The current UGI when the user initialize the		
51	   * client will be used to do the put and the delegation token operations. The		
52	   * current user may use {@link UserGroupInformation#doAs} another user to		
53	   * construct and initialize a timeline client if the following operations are		
54	   * supposed to be conducted by that user.		
55	   */
{code}
# TimelineClient Line 42, it should be @see TimelineClient. In the preceding line, maybe no need of space between time and line.


was (Author: varun_saxena):
# There is a bit of connection related duplicate code in TimelineClientImpl and TimelineClientV2Impl#serviceInit. I think we can turn TimelineClientHelper into a service (or a standalone class whose instance can be created) which can be initialized and stopped (to do the cleanup). We can encapsulate all Jersey client related stuff and connection retry logic in this class. If timeline client implementations want to access something from this class, we can provide relevant getters. I think this would be more cleaner.
# There are a lot of variables in both TimelineClientImpl and TimelineClientV2Impl which need not be class member variables as they are not referred to only during initialization.
# I agree in principle with the comment above i.e. to reduce the visibility of TimelineClient*Impl constructor. I think we can move the classes to same package as TimelineClient and TimelineV2Client. As TimelineClientImpl is annotated as Private, this should be fine, backward compatibility wise as well.
# In MRAppMaster#RunningAppContext, the comment over getTimelineClient method isn't suitable.
# ApplicationMaster Line 302, timelineV2Client need not be made visible for testing.
# We will reuse TimelineClientConnectionRetry for V2 as well so I think its fine to move it.
# TimelineV2Client, Line 35, we should say @see TimelineV2Client instead of TimelineClient
# In TimelineClient we had the below javadoc which has been removed in the patch. It was wrongly placed over contextAppId before. But the javadoc need not be removed. It should be placed over method createTimelineClient.
{code}
49	  /**
50	   * Create a timeline client. The current UGI when the user initialize the		
51	   * client will be used to do the put and the delegation token operations. The		
52	   * current user may use {@link UserGroupInformation#doAs} another user to		
53	   * construct and initialize a timeline client if the following operations are		
54	   * supposed to be conducted by that user.		
55	   */
{code}
# TimelineClient Line 42, it should be @see TimelineClient. In the preceding line, maybe no need of space between time and line.

> 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org