Vrushali C commented on YARN-3901:

Thanks [~djp] , appreciate the feedback! I too am figuring out how to make this 
more readable and sincerely appreciate everyone's time and efforts in reviewing 

Some responses:
bq. TestHBaseTimelineWriterImplFlowRun.java, Do we accept r1 to be null or 
empty result? I don't think so, so may be we should check the size of r1 
earlier so we are not ignore the real failure cases?

Right, I will add an assert not null here.

bq. In TimelineWriterUtils.java,
I think getIncomingAttributes() tries to clone an array of attributes with 
appending an extra attribute in AggregationOperations. May be we should have a 
javadoc to describe it. The 3 if else cases sounds unnecessary and can be 
I think I will update this method a bit more and add more comments so that it 
explains well what the code is doing.

bq. In ColumnHelper.java, Do we expect null element added to attributes? If 
not, we should complain with NPE or other exception instead of ignore it 
Hmm. So this method in ColumnHelper is called from several places and is nested 
between many calls from hbase writer till here. Since the list of attributes is 
a variable length list of parameters, it could be null or turn out to be empty 
if some function in between decides to remove an attribute, so this was more of 
a safety check. The list of Attributes can be modified at several places in the 
call stack, so it is not actually an error if it comes to this point as an 
empty list. But I will think over this a bit more. 

bq. For isApplicationFinished() and getApplicationFinishedTime(), the event 
list in TimelineEntity is a SortedSet, so we can use last() to retrieve the 
last event instead of for each every element? It shouldn't be other events 
after FINISHED_EVENT_TYPE. Isn't it?
Ah, I did not know that it was a sorted set, will update the code accordingly.

bq.  In addition, because the method is general enough. In addition, we can 
consider to move them to TimelineUtils class so can be reused by other classes. 
Sounds good, will refactor it.

Looks like the indentation is a bit off in some places, I will update the 
formatting as recommeded by [~gtCarrera9], [~jrottinghuis] and [~djp] 

> Populate flow run data in the flow_run & flow activity tables
> -------------------------------------------------------------
>                 Key: YARN-3901
>                 URL: https://issues.apache.org/jira/browse/YARN-3901
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Vrushali C
>            Assignee: Vrushali C
>         Attachments: YARN-3901-YARN-2928.1.patch, 
> YARN-3901-YARN-2928.2.patch, YARN-3901-YARN-2928.3.patch, 
> YARN-3901-YARN-2928.WIP.2.patch, YARN-3901-YARN-2928.WIP.patch
> As per the schema proposed in YARN-3815 in 
> https://issues.apache.org/jira/secure/attachment/12743391/hbase-schema-proposal-for-aggregation.pdf
> filing jira to track creation and population of data in the flow run table. 
> Some points that are being  considered:
> - Stores per flow run information aggregated across applications, flow version
> RM’s collector writes to on app creation and app completion
> - Per App collector writes to it for metric updates at a slower frequency 
> than the metric updates to application table
> primary key: cluster ! user ! flow ! flow run id
> - Only the latest version of flow-level aggregated metrics will be kept, even 
> if the entity and application level keep a timeseries.
> - The running_apps column will be incremented on app creation, and 
> decremented on app completion.
> - For min_start_time the RM writer will simply write a value with the tag for 
> the applicationId. A coprocessor will return the min value of all written 
> values. - 
> - Upon flush and compactions, the min value between all the cells of this 
> column will be written to the cell without any tag (empty tag) and all the 
> other cells will be discarded.
> - Ditto for the max_end_time, but then the max will be kept.
> - Tags are represented as #type:value. The type can be not set (0), or can 
> indicate running (1) or complete (2). In those cases (for metrics) only 
> complete app metrics are collapsed on compaction.
> - The m! values are aggregated (summed) upon read. Only when applications are 
> completed (indicated by tag type 2) can the values be collapsed.
> - The application ids that have completed and been aggregated into the flow 
> numbers are retained in a separate column for historical tracking: we don’t 
> want to re-aggregate for those upon replay

This message was sent by Atlassian JIRA

Reply via email to