Zhijie Shen commented on YARN-3051:

Varun, thanks for take over the reader interface patch. I noticed there's a 
param difference: Map<String, NameValueRelations>  metricFilters.  I'd like to 
recommend we don't support the binary relationship in the initial jira. My 
intial param means to filter the entities who contain the given metrics. We can 
file a separate jira to add binary relationship filtering, for metrics, config 
and info altogether. How do you think?

Here's a couple of comments about the patch:

1. Why do we need JsonSetter for the data model objects?

2. Can we prevent introducing the test oriented configurations into 
YarnConfiguration, which is part of api?

3. Is "getTimelineRecordFromJSON" required to be exposed. I'm a bit 
conservative to put the methods in api/common, which mean we need to keep 
supporting it.

4. Maybe {{Field}} is better to be the inner class of TimelineReader or 
TimelineEntity. Otherwise, the name  a bit vague about what it represents.

5. It seems to be better to implement FileSystemTimelineStorageImpl that 
implements both TimelineReader and TimelineWriter. One motivation is to reuse 
some code. A more critical problem is that FS reader and writer are not 
a) Writer should not have written the mapping into APP_FLOW_MAPPING_FILE.
b) Currently, when updating an entity, a new entity json will be appended into 
the same file, but this reader impl assumes one entity per file. We need to 
sync the behavior between them.

> [Storage abstraction] Create backing storage read interface for ATS readers
> ---------------------------------------------------------------------------
>                 Key: YARN-3051
>                 URL: https://issues.apache.org/jira/browse/YARN-3051
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>    Affects Versions: YARN-2928
>            Reporter: Sangjin Lee
>            Assignee: Varun Saxena
>         Attachments: YARN-3051-YARN-2928.003.patch, 
> YARN-3051-YARN-2928.03.patch, YARN-3051-YARN-2928.04.patch, 
> YARN-3051-YARN-2928.05.patch, YARN-3051.Reader_API.patch, 
> YARN-3051.Reader_API_1.patch, YARN-3051.Reader_API_2.patch, 
> YARN-3051.Reader_API_3.patch, YARN-3051.Reader_API_4.patch, 
> YARN-3051.wip.02.YARN-2928.patch, YARN-3051.wip.patch, YARN-3051_temp.patch
> Per design in YARN-2928, create backing storage read interface that can be 
> implemented by multiple backing storage implementations.

This message was sent by Atlassian JIRA

Reply via email to