hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Varun Saxena (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 18:30:13 GMT

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

Varun Saxena commented on YARN-4447:
------------------------------------

Thanks [~sjlee0] for the review.
bq. 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?
Spaces in the expression will have to be encoded while sending them in URL. Colons were being
used in previous expressions. If I am not wrong even in ATSv1. It should be fine in query
params I think. Colons are reserved characters only in the scheme section of URI as per my
understanding. java.net.URI does not seem to have any objections with colons in query param
either.
This is what RFC 3986 ABNF states :
{panel}
URI           = scheme ":" hier-part [ "?" query ] [ "#" fragment ]
query         = *( pchar / "/" / "?" )
{panel}
Here, query section does not have colon as reserved character.
However, I am not a 100% sure on this either.

bq. 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.
If you are talking about converting the whole expression to lower case, we cannot convert
the whole string to lowercase. Because configs/metrics/events/info will have to be taken as-is.
The reason being we do not convert them to lower case when we write them to backend.

bq. 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.
Sure, will add a note in javadoc. Had to make them mutable because I wanted to set the variables
as I was parsing the expression. Maybe you can have some alternate suggestion when you go
through the parsing code.

bq. 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?
You can say that. I moved this code out of parsing code I had initially written. 
The main reason for parsing it character by character was to reject invalid expressions whenever
an unexpected character based on parsing state is encountered. And yes, performance was a
consideration as well.
However for OR/AND similar effect can be achieved with indexOf, substr and trim, which although
makes it slightly more expensive operation compared to just checking character.  
I do understand though, it will be a negligible penalty.
Basically I just do not want to check for OR but also reject the expression if ORR comes in
place of OR. If code is confusing to read, I can consider changing it.
 
bq. Why did we move isIntegralValue() to TimelineReaderUtils?
Wanted to check if metric values can have non numerical and non integral values and if it
has reject it while parsing itself. We can defer the checking to storage layer as well though
and in that case it can be moved back to TimelineStorageUtils. Thoughts ?

bq. 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? 
Yes, thats correct. We have a JIRA for refactoring test cases(with regards to TestHBaseTimelineStorage).
We can take up refactoring of this test class as well there.


> 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