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