hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sangjin Lee (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-4447) Provide a mechanism to represent complex filters and parse them at the REST layer
Date Sat, 30 Apr 2016 17:23:12 GMT

    [ https://issues.apache.org/jira/browse/YARN-4447?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15265379#comment-15265379
] 

Sangjin Lee commented on YARN-4447:
-----------------------------------

Thanks [~varun_saxena]! This is a very comprehensive (and important) piece of work.

I'm trying to go over the patch, but I'm not quite sure if I can do a thorough job in a day
or two, given its size. Let me report some early feedback and questions first. The main things
I haven't fully gone through yet are the parser classes.

Do we have an escape/collision issue on the URL, or are they all properly encoded/decoded?
I'm concerned about colons and spaces specifically. Is it a non-issue?

In the parser code, I see a lot of {{Character.toLowerCase()}} operations. Since we need a
fully lower-cased input string anyway, I would say simply lower-case the input string and
operate directly on that. That might make the code less verbose with little performance impact.

It looks like {{TimelineFilter}}s (and subclasses) are now mutable. Then we need to be real
careful as they should not be used as keys for {{HashMap}}s or in {{HashSet}}s. This might
be a little paranoid, but it would be great if you could add a sentence in the javadoc that
these are not immutable and cannot be used in {{HashMap}}s or {{HashSet}}s.

(TimelineParseUtils.java)
- Why elaborate parsing based on chars? Can we not use {{expr.toLowerCase().indexOf("or ")
== offset}}? Was there a performance concern? Or was it simply refactored out of the parser
code that seems to be doing similar things?

(TimelineStorageUtils.java)
- Why did we move {{isIntegratlValue()}} to {{TimelineReaderUtils}}? From the name it sounds
like {{TimelineStorageUtils}} should be a common utility, and if both readers and writers
have a need, it should stay in {{TimelineStorageUtils}}?

(TimelineReaderWebServicesHBaseStorage.java)
- This is an observation that the size of this test is growing tremendously with this patch.
Would it be an option to refactor filter-related tests into a separate test class? It doesn't
have to be done as part of this JIRA, but it would be great if we took time to do it later.

> Provide a mechanism to represent complex filters and parse them at the REST layer 
> ----------------------------------------------------------------------------------
>
>                 Key: YARN-4447
>                 URL: https://issues.apache.org/jira/browse/YARN-4447
>             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: Timeline-Filters.pdf, YARN-4447-YARN-2928.01.patch
>
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message