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