Sangjin Lee commented on YARN-3862:

I had a chance to go over the latest patch in a little more detail. I think 
this is now closer to being ready. I do have some comments and suggestions, 
some major and others minor.

- createHBaseColQualPrefixFilter(): this is still trying to compute the column 
prefix by hand. The main point of introducing getColumnPrefixBytes() on 
ColumnPrefix was to avoid doing this for confs and metrics. Can we rework the 
signatures of createHBaseFilterList() so that we can rely on 
ColumnPrefix.getColumnPrefixBytes()? Ideally all computations of qualifier 
bytes should go through ColumnPrefix.getColumnPrefixBytes().

- I'm not too sure about the name; for other tests we basically combined the 
reader and writer tests. Thoughts on how to make this best fit into the 
existing tests?

- l.139: nit: typo: releated -> related
- I keep confusing configFilters and confs. The names are so similar that I 
have to go check the implementations to distinguish them (configFilters 
filtering rows we want to return, and confs filters contents of the matching 
rows). Could there be a better way to name them so that their meanings are 
clearer? I don't have a great idea at the moment, and you might want to think 
about better names...
- On a related note, this is probably outside the scope of this JIRA, but I see 
that the configFilter and metricFilter are applied on the client-side. Probably 
on a separate JIRA, we should see if we can do this on the HBase side. This is 
just a reminder.
- l.156: Why do we need to check if configFilters == null? Is it because if 
configFilters are specified we implicitly assume we want the config columns 
returned in the content? Is that a valid assumption?

- Related to one of the points above, at least we should add javadoc that 
clearly explains confs and metrics and how they are different from 
configFilters and metricFilters. That will help us a great deal in maintaining 

- As a result of YARN-4053 being committed, getColumnPrefixBytes(String) 
already exists. It should be removed from this patch.

- testWriteFlowRunMetricsPrefix() and testWriteFlowRunsMetricFields() are 
failing possibly due to changes in YARN-4053.

> Decide which contents to retrieve and send back in response in TimelineReader
> -----------------------------------------------------------------------------
>                 Key: YARN-3862
>                 URL: https://issues.apache.org/jira/browse/YARN-3862
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>    Affects Versions: YARN-2928
>            Reporter: Varun Saxena
>            Assignee: Varun Saxena
>              Labels: yarn-2928-1st-milestone
>         Attachments: YARN-3862-YARN-2928.wip.01.patch, 
> YARN-3862-YARN-2928.wip.02.patch, YARN-3862-YARN-2928.wip.03.patch, 
> YARN-3862-feature-YARN-2928.wip.03.patch
> Currently, we will retrieve all the contents of the field if that field is 
> specified in the query API. In case of configs and metrics, this can become a 
> lot of data even though the user doesn't need it. So we need to provide a way 
> to query only a set of configs or metrics.
> As a comma spearated list of configs/metrics to be returned will be quite 
> cumbersome to specify, we have to support either of the following options :
> # Prefix match
> # Regex
> # Group the configs/metrics and query that group.
> We also need a facility to specify a metric time window to return metrics in 
> a that window. This may be useful in plotting graphs 

This message was sent by Atlassian JIRA

Reply via email to