[
https://issues.apache.org/jira/browse/YARN-3863?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15166487#comment-15166487
]
Sangjin Lee commented on YARN-3863:
-----------------------------------
I completed one full pass over the patch (it's large!), and I wouldn't call it
complete yet. I may follow up with more comments as I delve more into it. I'd
welcome others' reviews too!
Here are the comments from this review.
(TimelineEntityFilters.java)
- l.48: typo: "a entity type" -> "the entity type"
- There are multiple places where a space is missing before an opening
parenthesis ("("). I also saw it in other files too. You want to have a space
before the opening parenthesis.
- l.51: make it a link
- l.59: typo: "a entity type" -> "the entity type"
- l.69: typo: "a info key" -> "the info key"
- l.81: make it a link
- l.91: make it a link
- l.99: make it a link
(TimelineReaderWebServicesUtils.java)
- l.94: I'm not really sure what this change is intended to do. The goal is to
do an equality filter against multiple values, right? Why do we need a separate
{{parseMetricsFilters()}} method for this? What's changed?
- l.257: Why is it GREATER_OR_EQUAL instead of EQUAL?
- This is more of a question. Is a list of multiple equality filters the same
as the multi-val equality filter? If not, how are they different?
(TimelineCompareFilter.java)
- nit: let's make the member variables final
(TimelineFilter.java)
- l.52: the name "MULTIVAL_EQUALITY" is bit confusing, and it took me a little
bit to see this means equality with an element in the set (I thought it was
multiple key-value equality). Is this essentially "in the set" comparison? I
wonder if there could be a better name? The same goes for
{{TimelineMultiValueEqualityFilter}}.
(TimelineFilterUtils.java)
- l.104: can {{createSingleColValueFiltersByRange()}} be refactored to call
{{createHBaseSingleColValueFilter()}}?
- l.107: dead code?
(HBaseTimelineWriterImpl.java)
- Is this basically improving the code by using the strongly typed methods for
bytes? As mentioned in a previous comment, these changes (this and
{{\*Column\*}} changes) seem orthogonal. Would it be possible to isolate these
changes from the main changes?
- l.448: it should simply be a {{else if}}
(TimelineStorageUtils.java)
- There are many place here and others where {{equals()}} is used to compare
enums. All the enum comparisons should use simply "==".
- see my previous comment about refactoring to make these methods simpler and
easier to read
(GenericEntityReader.java)
- l.260: I know this is happening deep inside the method, but it seems like a
bit of an anti-pattern that we have to reference whether something is an
application v. entity. There are multiple places in {{GenericEntityReader}} for
this (basically each place where {{ApplicationColumn\*}} is used). I know there
is already a precedent (I introduced it :(), but now it's gone full bloom. This
makes the line between {{GenericEntityReader}} and {{ApplicationEntityReader}}
quite blurry. Would it be possible to refactor these so that application
behavior goes into {{ApplicationEntityReader}}? I haven't thought through what
kind of refactoring would make that separation possible, but it would be great
if you could come up with ideas to retain separation between
{{GenericEntityReader}} and {{ApplicationEnttiyReader}}.
- l.532: This is an interesting point. Should we categorically disallow any
multi-entity reads without a filter? Is it an obvious requirement? I understand
we already set some default values (e.g. created time, etc.) so this might be a
moot point, but do we need to check for it when some defaults are set anyway?
(TestHBaseTimelineStorage.java)
- I think we went back and forth on this, but this test is getting real long
now. Should we consider breaking it up in some fashion? I think we originally
broke it up as a reader test and a writer test, and then combined them into one
again. Would there be some value in separating them (with possibly a common
base class)? Or we could break it down along different types of entities? I'm
open to ideas.
(TimelineExistsFilter.java)
- l.32-33: nit: make them final
(TimelineMultiValueEqualityFilter.java)
- The name is bit confusing (see above)
> 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-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)
