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

Varun Saxena edited comment on YARN-6027 at 2/13/17 8:49 PM:
-------------------------------------------------------------

Thanks [~rohithsharma] for the patch. Few comments.

# I think we can throw a BadRequestException instead of IOException in 
FlowActivityEntityReader. IOException will lead to HTTP 500 response.
# Nit: It should be doesn't in the message "fromId does't belongs to 
clusterId=" + clusterId". Also belongs should be belong
# Here maybe we can tell that timestamp bit of fromId is incorrect. Or maybe 
stick with the same message as earlier i.e. without exception message because 
it looks quite generic. "Invalid fromId has been provided. " + e.getMessage()
# Check the length of this split as well? {{String[] userToFlow = 
split\[2\].split("@");}}
# For javadoc of fromId, maybe we can say Defines flow *activity* entity id
# While we are at it, can you fix some of the checkstyle issues. No need to fix 
protected fields in based class as its just a test.
# What about escaping / in cluster ? Do we want to disallow / in cluster?


was (Author: varun_saxena):
Thanks [~rohithsharma] for the patch. Few comments.

# I think we can throw a BadRequestException instead of IOException in 
FlowActivityEntityReader. IOException will lead to HTTP 500 response.
# Nit: It should be doesn't in the message "fromId does't belongs to 
clusterId=" + clusterId". Also belongs should be belong
# Here maybe we can tell that timestamp bit of fromId is incorrect. Or maybe 
stick with the same message as earlier i.e. without exception message. "Invalid 
fromId has been provided. " + e.getMessage()
# Check the length of this split as well? {{String[] userToFlow = 
split\[2\].split("@");}}
# For javadoc of fromId, maybe we can say Defines flow *activity* entity id
# While we are at it, can you fix some of the checkstyle issues. No need to fix 
protected fields in based class as its just a test.
# What about escaping / in cluster ? Do we want to disallow / in cluster?

> Support fromid(offset) filter for /flows API
> --------------------------------------------
>
>                 Key: YARN-6027
>                 URL: https://issues.apache.org/jira/browse/YARN-6027
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Rohith Sharma K S
>            Assignee: Rohith Sharma K S
>              Labels: yarn-5355-merge-blocker
>         Attachments: YARN-6027-YARN-5355.0001.patch, 
> YARN-6027-YARN-5355.0002.patch
>
>
> In YARN-5585 , fromId is supported for retrieving entities. We need similar 
> filter for flows/flowRun apps and flow run and flow as well. 
> Along with supporting fromId, this JIRA should also discuss following points
> * Should we throw an exception for entities/entity retrieval if duplicates 
> found?
> * TimelieEntity :
> ** Should equals method also check for idPrefix?
> ** Does idPrefix is part of identifiers?



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to