[
https://issues.apache.org/jira/browse/YARN-3863?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15176999#comment-15176999
]
Sangjin Lee commented on YARN-3863:
-----------------------------------
I did another pass at the latest patch.
One high level question: am I correct in understanding that if a relations
filter is specified for example but relation was *not* specified as part of
fields to retrieve, we would try to fetch the relation? So, in a sense, would
specifying a filter override/modify the fields to retrieve behavior? If so, how
much additional complexity is added by trying to support that behavior? What if
we simply reject or ignore the filters if they do not match the fields to
retrieve? Would it make the implementation simpler or harder? To me, supporting
more contents even if the filters and the fields to retrieve are not consistent
seems very much optional, and I'm not sure if it is worth it especially if it
adds a lot more complexity. What do you think?
(TimelineEntityFilters.java)
- l.49: typo: "ids'" -> "id's" (also in l.60)
- l.62: should be a link for {{TimelineKeyValuesFilter}}
- For limit, createdTimeBegin, and createdTimeEnd, we're ensuring they can
never be null. In that vein, I think it might make sense to start using
{{long}} over {{Long}} as part of the method interface. Thoughts?
(TimelineCompareFilter.java)
- l.36: Is the default constructor useful at all? It doesn't sound like it if
key and value are empty/null. Should we remove it?
(TimelineKeyValuesFilter.java)
- l.34-36: nit: let's make them final
- l.55-56: super-nit: an empty line between the methods would be good
- l.68: another super-nit: the C-style equality pattern is not needed/helpful
in java; let's just do {{values == null}}
(GenericEntityReader.java)
- l.90: Do we need to check if {{getFilters()}} returns null? When I check all
callers of {{getFilters()}}, some check for null and some don't. It would be
good to make it clear and consistent either way.
- l.95: See above comment in {{TimelineEntityFilters.java}}. If we switch to
{{long}}, this becomes easier to understand (no need to reason about unboxing
yielding a NPE).
(ApplicationEntityReader.java)
- l.89: see above
(TimelineReaderWebServicesUtils.java)
- l.257: I'm still not sure I understand. Is this a temporary thing until
YARN-4447 is addressed? What if the metric value happens to be negative? That
would be a non-match, then?
(TimelineStorageUtils.java)
- l.466: {{equals()}} should be replaced with {{==}}
- l.591: same
- l.666: a comment here that states we're using the latest value of the metric
might be helpful
(ColumnHelper.java)
- I'm not sure if this is an issue or not, but I remember there were cases
where we join an empty string at the end to have the qualifier end with the
separator, and that was for a reason. I hope this patch did not change an
occurrence of that inadvertently.
(unit tests)
- I know [~vrushalic] had some thoughts on how to split this monolithic
{{TestHBaseTimelineStorage}}. It might be good to come to a consensus on how to
split it...
> Support complex filters in TimelineReader
> -----------------------------------------
>
> Key: YARN-3863
> URL: https://issues.apache.org/jira/browse/YARN-3863
> Project: Hadoop YARN
> Issue Type: Sub-task
> Affects Versions: YARN-2928
> Reporter: Varun Saxena
> Assignee: Varun Saxena
> Labels: yarn-2928-1st-milestone
> Attachments: YARN-3863-YARN-2928.v2.01.patch,
> YARN-3863-YARN-2928.v2.02.patch, YARN-3863-YARN-2928.v2.03.patch,
> YARN-3863-feature-YARN-2928.wip.003.patch,
> YARN-3863-feature-YARN-2928.wip.01.patch,
> YARN-3863-feature-YARN-2928.wip.02.patch,
> YARN-3863-feature-YARN-2928.wip.04.patch,
> YARN-3863-feature-YARN-2928.wip.05.patch
>
>
> Currently filters in timeline reader will return an entity only if all the
> filter conditions hold true i.e. only AND operation is supported. We can
> support OR operation for the filters as well. Additionally as primary backend
> implementation is HBase, we can design our filters in a manner, where they
> closely resemble HBase Filters.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
