[
https://issues.apache.org/jira/browse/YARN-4675?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15830892#comment-15830892
]
Li Lu commented on YARN-4675:
-----------------------------
Thanks [~Naganarasimha]! I took a look at the 003 patch. Not sure why but I
found some duplicated code in TimelineClientImpl with the newly introduced
helper and v2 impl. Detailed comments:
AMRMClient (and AMRMClientAsync):
- Maybe we'd like to be more clear about registerTimelineV2Client? This is
something new and quite different to v1 clients.
- Do we allow registering timeline clients when the AMRMClient's timeline
version is not set to 2? Maybe we should at least leave a warning/error there?
This can save some time when debugging a misconfigured cluster.
DistributedShell, ApplicationMaster.java:
DS is an app that we demo how to use a lot of YARN features, so maybe we want
to tidy up timeline client related code pieces a little bit...
- Can we unify all {{if}}s for publishing timeline event? We may want to have
centralized methods to dispatch timeline client calls to v1 and v2.
- Also, instead of checking if a timeline client is null, shall we use flags
like limelineServiceV2?
TimelineClient:
- Let's refer to TimelineV2Client in the Java doc for v2 use cases?
{code}
Creates an instance of the timeline v.1.x client.
{code}
- We may also want to update the class's javadoc to reflect API changes over
Hadoop 2. At least mention this class is for timeline v1.x ONLY.
TimelineV2Client:
- Same javadoc issue.
- Shall we close the constructor to protected since we've experienced some
unexpected calls to it in v1? Or at least add a testing only tag?
- Does client users need to know the context app id? If so, we may need to
slightly relax the visibility of getContextAppId?
- Why do we need a setter for context app id? Maybe we want to make this
information immutable for timeline clients? Do we allow reusing timeline v2
clients across multiple applications?
TimelineClientImpl:
- Why do we need RESOURCE_URI_STR_V2? We need to further polish
constructResURI as well.
- serviceRetryInterval is never used.
- Duplicated code for TimelineClientConnectionRetry and JerseyRetryFilter as
in TimelineServiceHelper.
- pollTimelineServiceAddress, initConnConfigurator never called? Duplicates
with V2.
- new ConnectionConfigurator in initSslConnConfigurator duplicates some code
in TimelineServiceHelper.
TimelineServiceHelper:
- There are two {{TimelineServiceHelper}}s in our codebase? One is really
trivial. Shall we merge them or eliminate one of them?
TimelineV2ClientImpl:
- connectionRetry is never used.
Not necessarily addressed in this JIRA, but to bring into attention: We have a
TimelineClient in YarnClientImpl. Shall we do this even though the cluster is
configured with ATS v2?
> 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]
