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