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] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
Date Mon, 27 Feb 2017 16:35:45 GMT

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

Varun Saxena edited comment on YARN-6027 at 2/27/17 4:35 PM:
-------------------------------------------------------------

Thanks [~rohithsharma] for the patch.
The approach looks fine to me. As we will use fromId for almost all of the row keys, getRowKeyAsString
will be required for almost all the row key implementations. Hence, although when I gave the
comment, I was only talking about an interface for row key converter, newly added RowKey interface
is also fine with me.

Few comments.
# RowKeyConverter interface's type parameter can extend RowKey. I guess it makes sense only
in this scenario. Right? Change javadoc accordingly if you agree to change this.
# Javadoc for fromId mentions flow runs. It should be flows. Infact it can be changed to...
# In TestRowKeys, we should use new CLUSTER, flow name and user for conversion which uses
TimelineReaderUtils#DEFAULT_DELIMITER_CHAR instead of Separator#QUALIFIER. Also mix it with
DEFAULT_SEPARATOR_CHAR
# Nit: FlowActivityEntityReader l.116, && should be combined with either the line
above or below.
{code}
If specified, retrieve the next set of flows from the given fromId. The set of flows retrieved
is inclusive of specified fromId. fromId should be taken from the value associated with FROM_ID
info key in flow entity response which was sent earlier.
{code}
# TimelineReaderUtils should be final class.
# FROMID_KEY should be annotated with VisibleForTesting
# I was wondering if we should throw exception mentioning Invalid fromId is specified from
within decode(String) method? Because although we use this for fromid now, it may not always
be the case. Maybe just catch the exception in FlowActivityEntityReader and then throw BadRequestException
with an an appropriate message there.
# Why not throw BadRequestException from newly added code in FlowActivityEntityReader?
# Javadoc and checkstyle issues seem fixable. Also some non-javadoc comments can be added
in FlowActivityRowKeyConverter to explain that default delimiter and separator chars are encoded.


was (Author: varun_saxena):
Thanks [~rohithsharma] for the patch.
The approach looks fine to me. As we will use fromId almost all of the row keys, getRowKeyAsString
will be required for almost all the row key implementations. Hence, although when I gave the
comment, I was only talking about an interface for row key converter, newly added RowKey interface
is also fine with me.

Few comments.
# RowKeyConverter interface's type parameter can extend RowKey. I guess it makes sense only
in this scenario. Right? Change javadoc accordingly if you agree to change this.
# Javadoc for fromId mentions flow runs. It should be flows. Infact it can be changed to...
# In TestRowKeys, we should use new CLUSTER, flow name and user for conversion which uses
TimelineReaderUtils#DEFAULT_DELIMITER_CHAR instead of Separator#QUALIFIER. Also mix it with
DEFAULT_SEPARATOR_CHAR
# Nit: FlowActivityEntityReader l.116, && should be combined with either the line
above or below.
{code}
If specified, retrieve the next set of flows from the given fromId. The set of flows retrieved
is inclusive of specified fromId. fromId should be taken from the value associated with FROM_ID
info key in flow entity response which was sent earlier.
{code}
# TimelineReaderUtils should be final class.
# FROMID_KEY should be annotated with VisibleForTesting
# I was wondering if we should throw exception mentioning Invalid fromId is specified from
within decode(String) method? Because although we use this for fromid now, it may not always
be the case. Maybe just catch the exception in FlowActivityEntityReader and then throw BadRequestException
with an an appropriate message there.
# Why not throw BadRequestException from newly added code in FlowActivityEntityReader?
# Javadoc and checkstyle issues seem fixable. Also some non-javadoc comments can be added
in FlowActivityRowKeyConverter to explain that default delimiter and separator chars are encoded.

> 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,
YARN-6027-YARN-5355.0003.patch, YARN-6027-YARN-5355.0004.patch, YARN-6027-YARN-5355.0005.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: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message