hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Varun Saxena (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (YARN-4675) Reorganize TimeClientImpl into TimeClientV1Impl and TimeClientV2Impl
Date Mon, 23 Jan 2017 15:34:26 GMT

    [ 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


Mime
View raw message