Junping Du commented on YARN-3034:

bq. As a further clarification, my problem is mainly on the test distributed 
shell. Right now we're using very ad hoc ways to set which version of timeline 
service we're using. Currently we're using test names to distinguish timeline 
V1 and V2, and since both versions work on the same port, we need to explicitly 
disable one version to use the other. Instead of doing this in the test script 
each time, I'd hope that there are some global settings/logic on the server 
side to decide which exact version of timeline service to launch. All the tests 
need to do is to check (and set) the version of active timeline service and 
launch the mini YARN cluster. It's a little bit off topic here so let move the 
rest discussion to YARN-3352. 
Thanks [~gtCarrera9] for clarifying more on this. Agree that we should have a 
more clean way to launch v1 and v2 service in unit test. May be launch both on 
different ports? Anyway, let's continue the discussion on YARN-3352.

Back to the latest patch, mostly looks fine to me. Two minor comments:
+  public static final String TIMELINE_SERVICE_VERSION = YARN_PREFIX
+      + "timeline-service.version";
Can we replace this with TIMELINE_SERVICE_PREFIX + "version" ?

+            YarnConfiguration.DEFAULT_TIMELINE_SERVICE_ENABLED)
+            && conf.getBoolean(
+                YarnConfiguration.SYSTEM_METRICS_PUBLISHER_ENABLED,
+                YarnConfiguration.DEFAULT_SYSTEM_METRICS_PUBLISHER_ENABLED)
+            && YarnConfiguration.TIMELINE_SERVICE_VERSION_ONE.equals(conf.get(
+                YarnConfiguration.TIMELINE_SERVICE_VERSION,
+                YarnConfiguration.DEFAULT_TIMELINE_SERVICE_VERSION));
equals => equalsIgnoreCase as user may input v1 or v2 (in lower case) which 
should also be accepted. Also, we should add a warning message log if user put 
something illegal here or it just get silient without any warn.

BTW, [~sjlee0] has a refactor patch on YARN-3333 which should get in quickly. 
This patch may need to rebase when that one is in.

> [Aggregator wireup] Implement RM starting its ATS writer
> --------------------------------------------------------
>                 Key: YARN-3034
>                 URL: https://issues.apache.org/jira/browse/YARN-3034
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Sangjin Lee
>            Assignee: Naganarasimha G R
>         Attachments: YARN-3034-20150312-1.patch, YARN-3034.20150205-1.patch, 
> YARN-3034.20150316-1.patch, YARN-3034.20150318-1.patch
> Per design in YARN-2928, implement resource managers starting their own ATS 
> writers.

This message was sent by Atlassian JIRA

Reply via email to