Junping Du commented on YARN-2302:

Thanks for the patch, [~zjshen]! A couple of comments so far:

In ApplicationHistoryServer.java,

+  protected TimelineDataManager timelineDataManager;
Better to be private, as it only get consumed within ApplicationHistoryServer.

    timelineACLsManager = createTimelineACLsManager(conf);
Looks like we don’t need timelineACLsManager anymore except initiating 
TimeLineDataManager. We can completely remove it (method and variable) after 
merge below
  protected TimelineACLsManager createTimelineACLsManager(Configuration conf) {
    return new TimelineACLsManager(conf);

  protected TimelineDataManager createTimelineDataManager(Configuration conf) {
    return new TimelineDataManager(timelineStore, timelineACLsManager);


  private TimelineDataManager createTimelineDataManager(Configuration conf) {
    return new TimelineDataManager(timelineStore, new 
The visibility of method should be private, as it is not get assumed outside of 
class. There are also some similar unnecessary protected methods around in this 
class, see if you want to do update here also or we can do it separately later.

In TimelineDataManager.java,
+      try {
+        if (existingEntity == null) {
+          injectOwnerInfo(entity, callerUGI.getShortUserName());
+        }
+      } catch (YarnException e) {
+        // Skip the entity which messes up the primary filter and record the
+        // error
+        LOG.warn("Skip the timeline entity: " + entityID + ", because "
+            + e.getMessage());
This exception sounds more serious than just a warn, so Log.error here may make 
more sense?

> Refactor TimelineWebServices
> ----------------------------
>                 Key: YARN-2302
>                 URL: https://issues.apache.org/jira/browse/YARN-2302
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Zhijie Shen
>            Assignee: Zhijie Shen
>         Attachments: YARN-2302.1.patch
> Now TimelineWebServices contains non-trivial logic to process the HTTP 
> requests, manipulate the data, check the access, and interact with the 
> timeline store.
> I propose the move the data-oriented logic to a middle layer (so called 
> TimelineDataManager), and TimelineWebServices only processes the requests, 
> and call TimelineDataManager to complete the remaining tasks.
> By doing this, we make the generic history module reuse TimelineDataManager 
> internally (YARN-2033), invoking the putting/getting methods directly. 
> Otherwise, we have to send the HTTP requests to TimelineWebServices to query 
> the generic history data, which is not an efficient way.

This message was sent by Atlassian JIRA

Reply via email to