Junping Du commented on YARN-3901:

Thanks Vrushali for updating the patch. This is a great/huge work which also 
means it may need more rounds of review and could receive much more criticize 
than the normal case. Thanks again for being patient.
After go through the code, I had a few comments (with omitted some duplicated 
ideas with Joep or Li's comments):

In HBaseTimelineWriterImpl.java,
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? 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. Also, need to fix some indentation issues in this 

In ColumnHelper.java,
+      for (Attribute attribute : attributes) {
+        if (attribute != null) {
+          p.setAttribute(attribute.getName(), attribute.getValue());
+        }
+      }
Do we expect null element added to attributes? If not, we should complain with 
NPE or other exception instead of ignore it silently.

In ColumnPrefix.java, Indentation issue in Javadoc.

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 didn't go to coprocessor classes quite deeply but I agree with Joep's above 
comments that it need more Javadoc to explain what are outstanding methods 
In FlowRunCoprocessor.java, getTagFromAttribute() sounds like we are using 
exception to differentiate normal case in matching string with enum elements. 
Can we improve it with using EnumUtils?

In AggregationCompactionDimension.java,
I think the only usage here is to provide a method getAttribute() which return 
an attribute object mixed with app_id (in byte array). If so, why we make this 
an enum class instead of a regular class as APPLICATION_ID is the only element? 
May be more straightforward way is to have a utility class to getAttribute() 

In AggregationOperations.java,
Indentation issues.

Haven't quite go through code around flow activity table, more comments should 
comes in my 2nd round review.

Some quick check on test code, for TestHBaseTimelineWriterImplFlowRun.java,
+      Result r1 = table1.get(g);
+      if (r1 != null && !r1.isEmpty()) {
+        Map<byte[], byte[]> values = r1.getFamilyMap(FlowRunColumnFamily.INFO
+            .getBytes());
+        assertEquals(2, r1.size());
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?

> 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