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

Zhijie Shen commented on YARN-2033:
-----------------------------------

[~djp], thanks for your comments. I addressed most of your comments in the new 
patch, and fix one bug I've found locally. Bellow are some response w.r.t to 
your concerns.

bq. Why do we need getApplication(appAttemptId.getApplicationId(), 
ApplicationReportField.NONE) here?

Because I want to check whether the application exists in the timeline store or 
not, before retrieving the application attempt information. If the application 
doesn't exist, we need to throw ApplicationNotFoundException.

BTW, in YARN-1250, getting app is going to be required for each API, because we 
need to check whether the user has access to this application or not.

bq. 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.

I rethink about the backward compatibility, and I think it's not good to reply 
on checking APPLICATION_HISTORY_STORE, because its default is already the 
FS-based history store. The users may use this store without explicitly setting 
it in their config file. Instead, I think it's more reasonable to check 
APPLICATION_HISTORY_ENABLED to determine whether the user is using old history 
store, because it is false by default. Unless the user sets it explicitly in 
the config file, he's not able to use the old history store. Therefore I 
changed the logic in YarnClientImpl, ApplicationHistoryServer, 
YarnMetricsPublisher to reply on APPLICATION_HISTORY_ENABLED for backward 
compatibility.

Per the suggestion, if if the old history service stack is used, a warn level 
log will be recorded. In addition, when APPLICATION_HISTORY_ENABLED = true, 
YarnMetricsPublisher cannot be enabled, preventing RMApplicationHistoryWriter 
and YarnMetricsPublisher writing the application history simultaneously.

bq. 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.

I agree the builder model should be more decent, but it seems that it needs to 
change XXXXReport classes, which currently use newInstance to construct the 
instance. Let's file a separate Jira to deal with building a big record with 
quite a few fields.

bq. We should replace "hadoop.tmp.dir" and "/yarn/timeline/generic-history" 
with constant string in YarnConfiguration. BTW, "hadoop.tmp.dir" may not be 
necessary?

This is because conf.get("hadoop.tmp.dir") cannot be determined in advance.

bq. 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.

ApplicationContext is actually not an RPC interface, but is used internally in 
the server daemons. We previously refactored the code and created such common 
interface for RM and GHS to source the application/attempt/container report(s) 
(although RM still pulls the information from RMContext directly, such that we 
could use the same CLI/webUI/service, but hook on different data source. 
Anyway, the annotations here are misleading, such that I deleted them.

bq. I am not sure if this change (and other changes in this class) is 
necessary. If not, we can remove it.

I did this intentionally. In fact, I wanted to discard
{code}
  protected int allocatedMB;
  protected int allocatedVCores;
{code}
Because history information doesn't include the runtime resource usage 
information. If we keep the two fields here, in the web services output, we 
will always see allocatedMB=0, and allocatedVCores=0.

bq. We already have the same implementation of MultiThreadedDispatcher in 
RMApplicationHistoryWriter.java.

That's right. Again it's duplicated by purpose. After this patch, I'm going to 
deprecate the classes of the old generic history read/write layer, including 
RMApplicationHistoryWriter (YARN-2320), such that in the next big release (e.g. 
Hadoop 3.0), we can remove the deprecated code. MultiThreadedDispatcher should 
be the sub-component of YarnMetricsPublisher unless it is going be used by 
other components. It it happens, we can promote it to the first-citizen class.

> 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.6.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