[ 
https://issues.apache.org/jira/browse/YARN-2033?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14099974#comment-14099974
 ] 

Junping Du commented on YARN-2033:
----------------------------------

[~zjshen], thanks for comments above which sounds good to me. I just go through 
your latest patch, a couple of comments:
{code}
+  public static final String 
RM_METRICS_PUBLISHER_MULTI_THREADED_DISPATCHER_POOL_SIZE =
+      RM_PREFIX + "metrics-publisher.multi-threaded-dispatcher.pool-size";
+  public static final int 
DEFAULT_RM_METRICS_PUBLISHER_MULTI_THREADED_DISPATCHER_POOL_SIZE =
+      10;
{code}
The name of config looks like too long. May be we can rename it to something 
shorter, i.e. RM_PREFIX + "metrics-publisher.dispatcher.pool-size"?

{code}
-  optional string diagnostics = 5 [default = "N/A"];
-  optional YarnApplicationAttemptStateProto yarn_application_attempt_state = 6;
-  optional ContainerIdProto am_container_id = 7;
+  optional string original_tracking_url = 5;
+  optional string diagnostics = 6 [default = "N/A"];
+  optional YarnApplicationAttemptStateProto yarn_application_attempt_state = 7;
+  optional ContainerIdProto am_container_id = 8;
{code}
We shouldn't insert a new field as it will change the order of existing fields. 
In PB, the encoded messages only include field type and number and will be map 
to field name when decoding. Thus, change the field number here will break 
compatibility which is unnecessary. Add original_tracking_url with field number 
of 8 should be fine.

{code}
-    if (conf.getBoolean(YarnConfiguration.APPLICATION_HISTORY_ENABLED,
-      YarnConfiguration.DEFAULT_APPLICATION_HISTORY_ENABLED)) {
-      historyServiceEnabled = true;
+    if (conf.get(YarnConfiguration.APPLICATION_HISTORY_STORE) == null
+        && conf.getBoolean(YarnConfiguration.RM_METRICS_PUBLISHER_ENABLED,
+            YarnConfiguration.DEFAULT_RM_METRICS_PUBLISHER_ENABLED) ||
+        conf.get(YarnConfiguration.APPLICATION_HISTORY_STORE) != null
+        && conf.getBoolean(YarnConfiguration.APPLICATION_HISTORY_ENABLED,
+            YarnConfiguration.DEFAULT_APPLICATION_HISTORY_ENABLED)) {
+      yarnMetricsEnabled = true;
{code}
If user's config is slightly wrong (let's assume: 
YarnConfiguration.APPLICATION_HISTORY_STORE != null, 
YarnConfiguration.RM_METRICS_PUBLISHER_ENABLED=true), then here we disable 
yarnMetricsEnabled sliently which make trouble-shooting effort a little harder. 
Suggest to log warn messages when user's wrong configuration happens. Better to 
move logic operations inside of if() to a separated method and log the error 
for wrong configuration. 

{code}
+  <property>
+       <description>The setting that controls whether yarn metrics is 
published on
+    the timeline server or not by RM.</description>
+       <name>yarn.resourcemanager.metrics-publisher.enabled</name>
+       <value>false</value>
+  </property>
{code}
Indentation should be 2 white spaces instead of tab.

In ApplicationHistoryManagerOnTimelineStore.java,
{code}
} catch (YarnException e) {
+      throw new IOException(e);
+    }
{code}
This kind of exception translate is unnecessary to me. We can remove it as 
YarnException get throw here. If we decide to throw IOException only (please 
see my comments later), we can extend the block to cover more code that could 
throw YarnException and translate to IOException.

The method of convertToApplicationReport seems a little too sophisticated in 
creating applicationReport. Another option is to wrapper it as Builder pattern 
(plz refer in MiniDFSCluster) should be better. The same comments on 
convertToApplicationAttemptReport and convertToContainerReport. This is only 
optional comments, see if you want to address here or some separate JIRA in 
future.

{code}
+  public ApplicationAttemptReport getApplicationAttempt(
+      ApplicationAttemptId appAttemptId) throws YarnException, IOException {
+    getApplication(appAttemptId.getApplicationId(), 
ApplicationReportField.NONE);
+    TimelineEntity entity = null;
...
{code}
Why do we need getApplication(appAttemptId.getApplicationId(), 
ApplicationReportField.NONE) here? IMO, the only work here is to check if 
applicationId is valid, but we have check on appAttemptId later. so we may 
consider to remove it if unnecessary. In addition, may be 
ApplicationReportField.NONE is not useful?

{code}
-        new Path(conf.get(YarnConfiguration.FS_APPLICATION_HISTORY_STORE_URI));
+        new Path(conf.get(YarnConfiguration.FS_APPLICATION_HISTORY_STORE_URI,
+            conf.get("hadoop.tmp.dir") + "/yarn/timeline/generic-history"));
{code}
We should replace "hadoop.tmp.dir" and "/yarn/timeline/generic-history" with 
constant string in YarnConfiguration. BTW, "hadoop.tmp.dir" may not be 
necessary?

In ApplicationContext.java,
{code}
    * @return {@link ApplicationReport} for the ApplicationId.
+   * @throws YarnException
    * @throws IOException
    */
   @Public
   @Unstable
-  ApplicationReport getApplication(ApplicationId appId) throws IOException;
+  ApplicationReport getApplication(ApplicationId appId)
+      throws YarnException, IOException;
{code}
For public API (although marked as unstable), adding a new exception will break 
compatibility of RPC as old version client don't know how to deal with new 
exception. We'd better to keep the compatibility here with throwing IOException 
only and translating YarnExcetption to IOException inside server side.

In AppInfo.java,
{code}
-    ApplicationResourceUsageReport usage =
-        app.getApplicationResourceUsageReport();
-    if (usage != null) {
-      allocatedMB = usage.getUsedResources().getMemory();
-      allocatedVCores = usage.getUsedResources().getVirtualCores();
-    }
{code}
I am not sure if this change (and other changes in this class) is necessary. If 
not, we can remove it.

In YarnMetricsPublisher.java,
{code}
+  @SuppressWarnings({ "rawtypes", "unchecked" })
+  protected static class MultiThreadedDispatcher extends CompositeService
+      implements Dispatcher {
+
+    private List<AsyncDispatcher> dispatchers =
+        new ArrayList<AsyncDispatcher>();
{code}
We already have the same implementation of MultiThreadedDispatcher in 
RMApplicationHistoryWriter.java. We should consolidate two to an indepedent 
class rather than two inner classes.

More comments may comes later.

> Investigate merging generic-history into the Timeline Store
> -----------------------------------------------------------
>
>                 Key: YARN-2033
>                 URL: https://issues.apache.org/jira/browse/YARN-2033
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Vinod Kumar Vavilapalli
>            Assignee: Zhijie Shen
>         Attachments: ProposalofStoringYARNMetricsintotheTimelineStore.pdf, 
> YARN-2033.1.patch, YARN-2033.2.patch, YARN-2033.3.patch, YARN-2033.4.patch, 
> YARN-2033.5.patch, YARN-2033.Prototype.patch, YARN-2033_ALL.1.patch, 
> YARN-2033_ALL.2.patch, YARN-2033_ALL.3.patch, YARN-2033_ALL.4.patch
>
>
> Having two different stores isn't amicable to generic insights on what's 
> happening with applications. This is to investigate porting generic-history 
> into the Timeline Store.
> One goal is to try and retain most of the client side interfaces as close to 
> what we have today.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Reply via email to