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

Junping Du commented on YARN-4234:
----------------------------------

Thanks [~xgong] for updating the patch. I just go through the latest patch, and 
below is my review comments:

In TimelineEntityGroupId.java,
Can we add Javadoc for TimelineEntityGroupId and explain what 
TimelineEntityGroupId is used for?
Also in TimelineEntityGroupId.equals(), can we omit unnecessary ";"?

In FileSystemTimelineWriter.java,
{noformat}
      LOG.debug(
          YarnConfiguration.TIMELINE_SERVICE_CLIENT_FD_FLUSH_INTERVAL_SECS
              + "=" + flushIntervalSecs + ", " +
          YarnConfiguration.TIMELINE_SERVICE_CLIENT_FD_CLEAN_INTERVAL_SECS
              + "=" + cleanIntervalSecs + ", " +
          YarnConfiguration.TIMELINE_SERVICE_CLIENT_FD_RETAIN_SECS
              + "=" + ttl + ", " +
          TIMELINE_SERVICE_ENTITYFILE_FS_SUPPORT_APPEND
              + "=" + isAppendSupported);
{noformat}
Shall we log other related configurations, like: activePath, retryPolicy, 
summaryEntityTypes, etc.?

In putEntities(),
The name of entitiesToSummary, entitiesToEntity and entitiesToDB sounds a 
little confusing. How about update them to entitiesToSummaryCache, 
entitiesToEntityCache and entitiesToDBStore? Also, we shouldn't log in INFO 
level for each entity get written in cache. Debug level should be good.

In LogFDsCache,
{noformat}
      summanyLogFDs = new HashMap<ApplicationAttemptId, EntityLogFD>();
      entityLogFDs = new HashMap<ApplicationAttemptId,
          HashMap<TimelineEntityGroupId, EntityLogFD>>();
{noformat}
There is a race condition here: if LogFDsCache is doing flush with iterating 
the map and some writing entity/summary come to insert the HashMap at the same 
time, then it will throw ConcurrentModificationException. We should use 
ConcurrentHashMap instead.
Also, for CleanInActiveFDsTask, We should use WARN level for LOG instead of 
DEBUG in case exception get throw.

In TimelineWriter.java,
The conf seems not get used. If so, we can remove it from class and constructor?

Other looks good to me.

> New put APIs in TimelineClient for ats v1.5
> -------------------------------------------
>
>                 Key: YARN-4234
>                 URL: https://issues.apache.org/jira/browse/YARN-4234
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Xuan Gong
>            Assignee: Xuan Gong
>         Attachments: YARN-4234-2015-11-13.1.patch, 
> YARN-4234-2015-11-16.1.patch, YARN-4234-2015-11-16.2.patch, 
> YARN-4234-20151111.2.patch, YARN-4234.1.patch, YARN-4234.2.patch, 
> YARN-4234.2015-11-12.1.patch, YARN-4234.2015-11-12.1.patch, 
> YARN-4234.2015-11-18.1.patch, YARN-4234.2015-11-18.2.patch, 
> YARN-4234.2015-12-09.patch, YARN-4234.2015-12-09.patch, 
> YARN-4234.20151109.patch, YARN-4234.20151110.1.patch, 
> YARN-4234.20151111.1.patch, YARN-4234.3.patch
>
>
> In this ticket, we will add new put APIs in timelineClient to let 
> clients/applications have the option to use ATS v1.5



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to