[
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: [email protected]
For additional commands, e-mail: [email protected]
