Zhijie Shen commented on YARN-3904:

[~gtCarrera9], thanks for the patch. Bellow are my comments:

bq. The two failed tests passed on my local machine, and the failures appeared 
to be irrelevant. This said, we may still need to fix those intermittent test 

Do we plan to fix it in this patch?

Some high level comments:

1. As is also mentioned in YARN-3049, how about we refactoring reader/writer 
method signature in a separate jira to avoid conflicts?

2.  I suggest moving the table creation stuff into TimelineSchemaCreator.

3. As HBase backend is accessed both directly and via Phoenix, it's good for us 
to cleanup the configuration to say we're using the HBase backend (comparing to 
FS backend) instead of specifically HBase or Phoenix writer/reader.

Other patch details:

1. Make OfflineAggregationWriter extend Service, such that you don't need to 
define init.

2. Now we're working towards a production standard patch. Would you please 
write some javadoc to explain the schema of the aggregation tables like what we 
did for HBase tables.

3. The connection config should be moved to YarnConfiguration.

4. Why is info column family kept? I expect the aggregation table will only 
have metrics data

5. Let's also have a default PhoenixOfflineAggregationWriterImpl constructor to 
be used in the production code.

6. {{Class.forName(DRIVER_CLASS_NAME);}} doesn't  need to be invoked every time 
we get a connection.

> Refactor timelineservice.storage to add support to online and offline 
> aggregation writers
> -----------------------------------------------------------------------------------------
>                 Key: YARN-3904
>                 URL: https://issues.apache.org/jira/browse/YARN-3904
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Li Lu
>            Assignee: Li Lu
>         Attachments: YARN-3904-YARN-2928.001.patch, 
> YARN-3904-YARN-2928.002.patch, YARN-3904-YARN-2928.003.patch, 
> YARN-3904-YARN-2928.004.patch, YARN-3904-YARN-2928.005.patch
> After we finished the design for time-based aggregation, we can adopt our 
> existing Phoenix storage into the storage of the aggregated data. In this 
> JIRA, I'm proposing to refactor writers to add support to aggregation 
> writers. Offline aggregation writers typically has less contextual 
> information. We can distinguish these writers by special naming. We can also 
> use CollectorContexts to model all contextual information and use it in our 
> writer interfaces. 

This message was sent by Atlassian JIRA

Reply via email to