[
https://issues.apache.org/jira/browse/YARN-4356?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15049817#comment-15049817
]
Sangjin Lee commented on YARN-4356:
-----------------------------------
Thanks [~djp] for your comments. I addressed most of your comments in the new
patch. The following is a response to your comments.
bq. We should also add back types as requirement/convension for generics.
This is the java 7 diamond operator (<>) which is a shorthand for inferring
types. The type information is NOT removed. It's inferred by the compiler, and
the compiler produces the same bytecode as specifying the types.
bq. This check of null is unnecessary as the only caller - NMCollectorService
is only running under v2 is enabled. If for some reason, we get NPE here which
is still better than we ignore it silently.
That's a good catch. I agree that it's a little better not to check for null
here. It's changed in the latest patch.
bq. Please remove this unrelated change out for more focus and better tracking.
Agreed. I originally removed it because it was an unused private method, but I
put it back in.
bq. Just a reminder, keepAliveApps is a wrong list to identify running apps on
specific node. YARN-3586 (with patch) is already filed to fix this. We can
either merge that patch in or rebase that patch when this patch done.
Got it. Can we proceed with the current patch and get that fix once YARN-3586
goes in?
{quote}
In TimelineServiceV2Publisher.java,
- * This class is responsible for posting application, appattempt & Container
+ * This class is responsible for posting application, appattempt &
Container
Why we need this change?
{quote}
This is addressing a javadoc error. The ampersand ("&") is a special character
for javadoc, and it breaks javadoc. It needs to be entity-escaped:
{noformat}
[ERROR]
/testptch/hadoop/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/metrics/TimelineServiceV2Publisher.java:61:
error: bad HTML entity
[ERROR] * This class is responsible for posting application, appattempt &
Container
{noformat}
bq. We should disable PerNodeTimelineCollectorsAuxService if we don't enable
timeline service v2. Isn't it? If so, I think this is not a necessary change
and we should remove.
This is used for the test method launchServer(). This method is invoked
directly by a unit test (thus the @VisibleForTesting annotation). The same for
TimelineReaderServer.
bq. In addition, I think we should split the change that duplicated with
YARN-3623 and cherry-pick it from trunk/branch-2 when that patch get commit in.
That's fine. I still put up the patch that includes a version of that because
without it things won't even compile. I will wait until YARN-3623 goes in
before I remove that piece from this patch, then this can get committed.
Let me know if this answers your questions.
> ensure the timeline service v.2 is disabled cleanly and has no impact when
> it's turned off
> ------------------------------------------------------------------------------------------
>
> Key: YARN-4356
> URL: https://issues.apache.org/jira/browse/YARN-4356
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: timelineserver
> Affects Versions: YARN-2928
> Reporter: Sangjin Lee
> Assignee: Sangjin Lee
> Priority: Critical
> Labels: yarn-2928-1st-milestone
> Attachments: YARN-4356-feature-YARN-2928.002.patch,
> YARN-4356-feature-YARN-2928.003.patch, YARN-4356-feature-YARN-2928.004.patch,
> YARN-4356-feature-YARN-2928.005.patch,
> YARN-4356-feature-YARN-2928.poc.001.patch
>
>
> For us to be able to merge the first milestone drop to trunk, we want to
> ensure that once disabled the timeline service v.2 has no impact from the
> server side to the client side. If the timeline service is not enabled, no
> action should be done. If v.1 is enabled but not v.2, v.1 should behave the
> same as it does before the merge.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
