Sangjin Lee commented on YARN-4179:

- l.143-151: Why is this conversion from long to {{Date}} needed? I thought we 
always set a {{Date}} object via {{setDate}}? Did you see a case where a long 
was set? On a related note, do you think it would be easier if we set the long 
value (unix epoch) instead of the Date object to begin with? Might that make 
things a little easier? But one way or another, I don't think we need branching 
code here when we control these instances via {{setDate}}.
- l.144: nit (assuming this change is needed): let's use a normal java 
predicate style {{date != null}}

- l.81: As I mentioned in an earlier comment, can we simply use JDK's date 
formatter for this? It would be good to avoid adding a new dependency unless it 
is absolutely necessary.
- l.82: now the question of a more "standard" date format can be highly 
subjective, but how about using "yyyyMMdd"?
- l.88: It would be good to document in javadoc explicitly (see below for a 
little more discussion) what is an allowed date range (here and perhaps in the 
actual REST API methods).
- l.97: nit: {{parseDate}} can return primitive long
- l.107: I see that if the start date is missing we're ignoring the date range 
param altogether. Is this reasonable? Could there be a legitimate use case 
where one specifies only the end date (perhaps along with a limit)? Should we 
not support it?
- l.115: I'm a little confused by this. If the end date is missing, shouldn't 
we assume that it is open-ended (all the way to now)? Or are we interpreting 
that as to mean the user wants that specific date and that date only? This also 
shows that we need more comments here to specify the behavior explicitly. :)

- l.406: instead of duplicating the date format here, we should simply reuse 
what's defined in TimelineReaderWebServices, making that variable public or 

> [reader implementation] support flow activity queries based on time
> -------------------------------------------------------------------
>                 Key: YARN-4179
>                 URL: https://issues.apache.org/jira/browse/YARN-4179
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>    Affects Versions: YARN-2928
>            Reporter: Sangjin Lee
>            Assignee: Varun Saxena
>            Priority: Minor
>         Attachments: YARN-4179-YARN-2928.01.patch
> This came up as part of YARN-4074 and YARN-4075.
> Currently the only query pattern that's supported on the flow activity table 
> is by cluster only. But it might be useful to support queries by cluster and 
> certain date or dates.

This message was sent by Atlassian JIRA

Reply via email to