[jira] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
[
https://issues.apache.org/jira/browse/YARN-6027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15891235#comment-15891235
]
Sangjin Lee edited comment on YARN-6027 at 3/1/17 10:48 PM:
The YARN-5355-branch-2 branch is failing compilation at
{{AbstractTimelineReaderHBaseTestBase.java}}:
{noformat}
[ERROR] Failed to execute goal
org.apache.maven.plugins:maven-compiler-plugin:3.1:testCompile
(default-testCompile) on project
hadoop-yarn-server-timelineservice-hbase-tests: Compilation failure:
Compilation failure:
[ERROR]
/Users/sjlee/git/hadoop-ats/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-hbase-tests/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/AbstractTimelineReaderHBaseTestBase.java:[90,9]
method does not override or implement a method from a supertype
[ERROR]
/Users/sjlee/git/hadoop-ats/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-hbase-tests/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/AbstractTimelineReaderHBaseTestBase.java:[122,29]
cannot find symbol
[ERROR] symbol: method getStatusInfo()
[ERROR] location: variable resp of type com.sun.jersey.api.client.ClientResponse
[ERROR]
/Users/sjlee/git/hadoop-ats/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-hbase-tests/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/AbstractTimelineReaderHBaseTestBase.java:[126,34]
cannot find symbol
[ERROR] symbol: method getStatusInfo()
[ERROR] location: variable resp of type com.sun.jersey.api.client.ClientResponse
[ERROR]
/Users/sjlee/git/hadoop-ats/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-hbase-tests/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/AbstractTimelineReaderHBaseTestBase.java:[140,13]
cannot find symbol
[ERROR] symbol: method getStatusInfo()
[ERROR] location: variable resp of type com.sun.jersey.api.client.ClientResponse
{noformat}
I'll post an addendum patch for the branch-2 commit.
was (Author: sjlee0):
The YARN-5355-branch-2 branch is failing compilation at
{{AbstractTimelineReaderHBaseTestBase.java}}:
{noformat}
[ERROR] Failed to execute goal
org.apache.maven.plugins:maven-compiler-plugin:3.1:testCompile
(default-testCompile) on project
hadoop-yarn-server-timelineservice-hbase-tests: Compilation failure:
Compilation failure:
[ERROR]
/Users/sjlee/git/hadoop-ats/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-hbase-tests/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/AbstractTimelineReaderHBaseTestBase.java:[90,9]
method does not override or implement a method from a supertype
[ERROR]
/Users/sjlee/git/hadoop-ats/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-hbase-tests/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/AbstractTimelineReaderHBaseTestBase.java:[122,29]
cannot find symbol
[ERROR] symbol: method getStatusInfo()
[ERROR] location: variable resp of type com.sun.jersey.api.client.ClientResponse
[ERROR]
/Users/sjlee/git/hadoop-ats/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-hbase-tests/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/AbstractTimelineReaderHBaseTestBase.java:[126,34]
cannot find symbol
[ERROR] symbol: method getStatusInfo()
[ERROR] location: variable resp of type com.sun.jersey.api.client.ClientResponse
[ERROR]
/Users/sjlee/git/hadoop-ats/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-hbase-tests/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/AbstractTimelineReaderHBaseTestBase.java:[140,13]
cannot find symbol
[ERROR] symbol: method getStatusInfo()
[ERROR] location: variable resp of type com.sun.jersey.api.client.ClientResponse
{noformat}
I'll file a small JIRA to fix it.
> 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
> Fix For: YARN-5355, YARN-5355-branch-2
>
> 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,
> YARN-6027-YARN-5355.0006.patch, YARN-6027-YARN-5355.0007.patch,
> YARN-6027-YARN-5355.0008.patch
>
>
> In YARN-5585 , fromId is supported for retrieving entities. We need similar
> filter for flows/flowRun apps and flow run and flow as
[jira] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
[ https://issues.apache.org/jira/browse/YARN-6027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15887442#comment-15887442 ] Varun Saxena edited comment on YARN-6027 at 2/28/17 7:12 AM: - bq. Any converter class that does both can implement both separate interfaces. To be honest, this is what I meant when I first made the comment about separating this out into an interface. But after seeing Rohith's patch thought that as all row key converters will be requiring encoding/decoding string row key variant so let us go with what's in the patch. However, your point about this being orthogonal to current key version. And this being more about conversion to/from String makes sense to me. So let us go with 2 separate interfaces. bq. In terms of separator characters, I am of a little different opinion from Varun's. IMO it would be better if we could reuse the same separator character (Separator.QUALIFIERS) as a constant (not a new constant with the same value). Actually, the current patch does not use Separator enum for from id conversion. It uses 2 constants. My comment was more about using these constants in tests. However, I did think of reusing Separator enum. But how do we fit in the escape char? Move ESCAPE char also to Separator enum? Probably can be done but then it will not be used at all in methods defined in Separator enum. And picking one from Separator enum and other from a constant I thought would be weird. Also, I thought we will be mixing it up because for FROM_ID we would want URL safe characters as separator and escape characters because this will be carried in URL in eventually. Whereas Separator class is primarily used for conversion fo HBase bytes. So if somebody wishes to change separator in HBase row key, would he have to take care of this being URL safe character as well? We can probably leave a comment but then it would be somewhat restrictive. I had even thought of moving split and join methods used for String escaping to Separator enum but Separator enum is defined at the storage layer and these methods are also used for UID conversion. This was the thought process behind not suggesting reuse of Separator#QUALIFIERS. Your thoughts on the same? cc [~sjlee0] was (Author: varun_saxena): bq. Any converter class that does both can implement both separate interfaces. To be honest, this is what I meant when I first made the comment about separating this out into an interface. But after seeing Rohith's patch thought that as all row key converters will be requiring encoding/decoding string row key variant so let us go with what's in the patch. However, your point about this being orthogonal to current key version. And this being more about conversion to/from String makes sense to me. So let us go with 2 separate interfaces. bq. In terms of separator characters, I am of a little different opinion from Varun's. IMO it would be better if we could reuse the same separator character (Separator.QUALIFIERS) as a constant (not a new constant with the same value). Actually, the current patch does not use Separator enum for from id conversion. It uses 2 constants. My comment was more about using these constants in tests. However, I did think of reusing Separator enum. But how do we fit in the escape char? Move ESCAPE char also to Separator enum? Probably can be done but then it will not be used at all in Separator enum. Picking one from Separator enum and other from a constant I thought would be weird. Also, I thought we will be mixing it up because for FROM_ID we would want URL safe characters as separator and escape characters because this will be carried in URL in eventually. Whereas Separator class is primarily used for conversion fo HBase bytes. So if somebody wishes to change separator in HBase row key, would he have to take care of this being URL safe character as well? We can probably leave a comment but then it would be somewhat restrictive. I had even thought of moving split and join methods used for String escaping to Separator enum but Separator enum is defined at the storage layer and these methods are also used for UID conversion. This was the thought process behind not suggesting reuse of Separator#QUALIFIERS. Your thoughts on the same? > 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 >
[jira] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
[ https://issues.apache.org/jira/browse/YARN-6027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15887442#comment-15887442 ] Varun Saxena edited comment on YARN-6027 at 2/28/17 7:09 AM: - bq. Any converter class that does both can implement both separate interfaces. To be honest, this is what I meant when I first made the comment about separating this out into an interface. But after seeing Rohith's patch thought that as all row key converters will be requiring encoding/decoding string row key variant so let us go with what's in the patch. However, your point about this being orthogonal to current key version. And this being more about conversion to/from String makes sense to me. So let us go with 2 separate interfaces. bq. In terms of separator characters, I am of a little different opinion from Varun's. IMO it would be better if we could reuse the same separator character (Separator.QUALIFIERS) as a constant (not a new constant with the same value). Actually, the current patch does not use Separator enum for from id conversion. It uses 2 constants. My comment was more about using these constants in tests. However, I did think of reusing Separator enum. But how do we fit in the escape char? Move ESCAPE char also to Separator enum? Probably can be done but then it will not be used at all in Separator enum. Picking one from Separator enum and other from a constant I thought would be weird. Also, I thought we will be mixing it up because for FROM_ID we would want URL safe characters as separator and escape characters because this will be carried in URL in eventually. Whereas Separator class is primarily used for conversion fo HBase bytes. So if somebody wishes to change separator in HBase row key, would he have to take care of this being URL safe character as well? We can probably leave a comment but then it would be somewhat restrictive. I had even thought of moving split and join methods used for String escaping to Separator enum but Separator enum is defined at the storage layer and these methods are also used for UID conversion. This was the thought process behind not suggesting reuse of Separator#QUALIFIERS. Your thoughts on the same? was (Author: varun_saxena): bq. Any converter class that does both can implement both separate interfaces. To be honest, this is what I meant when I first made the comment about separating this out into an interface. But after seeing Rohith's patch thought that as all row key converters will be requiring encoding/decoding string row key variant so let us go with what's in the patch. However, your point about this being orthogonal to current key version. And this being more about conversion to/from String makes sense to me. So let us go with 2 separate interfaces. bq. In terms of separator characters, I am of a little different opinion from Varun's. IMO it would be better if we could reuse the same separator character (Separator.QUALIFIERS) as a constant (not a new constant with the same value). Actually, the current patch does not use Separator enum for from id conversion. It uses 2 constants. My comment was more about using these constants in tests. However, I did think of reusing Separator enum. But how do we fit in the escape char? Move ESCAPE char also to Separator enum? Probably can be done but then it will not be used at all in Separator enum. Picking one from Separator enum and other from a constant I thought would be weird. Also, I thought we will be mixing it up because for FROM_ID we would want URL safe characters as separator and escape characters because this will be carried in URL in eventually. Whereas Separator class is primarily used for conversion fo HBase bytes. So if somebody wishes to change separator in HBase row key, would he have to take care of this being URL safe character as well? We can probably leave a comment but then it would be somewhat restrictive. This was the thought process behind not suggesting reuse of Separator#QUALIFIERS. Your thoughts on the same? > 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
[jira] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
[
https://issues.apache.org/jira/browse/YARN-6027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15886195#comment-15886195
]
Varun Saxena edited comment on YARN-6027 at 2/27/17 5:51 PM:
-
[~rohithsharma],
bq. both are same right? any issues will happen because of enum? I debug the
code, the value is ! itself in cluster. May be need to add more combination of
tests.
They are both same right now. But what if somebody changes them in future? The
variables/enums used are different. FlowActivityRowKeyConverter#decode(byte[])
uses Separator#QUALIFIERS whereas FlowActivityRowKeyConverter#decode(String)
uses the constants defined in TimelineReaderUtils. Same goes for encode methods.
If somebody in future changes either one of them i.e. changes QUALIFIERS but
not SEPARATOR_CHAR to something other than "!", the intention of test case
won't be served. So it would be better to use the respective constant/enum as
per the converter method being invoked. This was the intention behind comment.
bq. IIUC, public static final constants need not to be annotated
Sorry, my bad. We do use FROM_ID outside the scope of tests and
TimelineReaderUtils.
bq. RowKeyConvertor interface extended from KeyConvertor interface. RowKey is
interface which is implemented by HBase table row key class . Both
RowKeyConvertor and RowKey interfaces are different.
What I meant was to define RowKeyConverter interface as under. We do not expect
RowKeyConverter to be extended for anything other than type parameter being
subtype of RowKey. Right? As your javadoc also says this interface has to be
implemented *for encoding and decoding row keys only*. Defining it like below
would sort of enforce this condition if all row keys implement RowKey interface.
{code}
public interface RowKeyConverter extends KeyConverter
{code}
bq. Even I got doubt while removing final, why it should be final? utils class
should be public
utils classes are not expected to be subclassed. Right? They just encapsulate a
bunch of static methods and constants.
was (Author: varun_saxena):
[~rohithsharma],
bq. both are same right? any issues will happen because of enum? I debug the
code, the value is ! itself in cluster. May be need to add more combination of
tests.
They are both same right now. But what if somebody changes them in future? The
variables/enums used are different. FlowActivityRowKeyConverter#decode(byte[])
uses Separator#QUALIFIERS whereas FlowActivityRowKeyConverter#decode(String)
uses the constants defined in TimelineReaderUtils. Same goes for encode methods.
If somebody in future changes either one of them i.e. changes QUALIFIERS but
not SEPARATOR_CHAR to something other than "!", the intention of test case
won't be served. So it would be better to use the respective constant/enum as
per the converter method being invoked. This was the intention behind comment.
bq. IIUC, public static final constants need not to be annotated
Sorry, my bad. We do use FROM_ID outside the scope of tests and
TimelineReaderUtils.
bq. RowKeyConvertor interface extended from KeyConvertor interface. RowKey is
interface which is implemented by HBase table row key class . Both
RowKeyConvertor and RowKey interfaces are different.
What I meant was to define RowKeyConverter interface as under. We do not expect
RowKeyConverter to be extended for anything other than type parameter being
subtype of RowKey. Right?
{code}
public interface RowKeyConverter extends KeyConverter
{code}
bq. Even I got doubt while removing final, why it should be final? utils class
should be public
utils classes are not expected to be subclassed. Right? They just encapsulate a
bunch of static methods and constants.
> 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,
[jira] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
[
https://issues.apache.org/jira/browse/YARN-6027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15886195#comment-15886195
]
Varun Saxena edited comment on YARN-6027 at 2/27/17 5:42 PM:
-
[~rohithsharma],
bq. both are same right? any issues will happen because of enum? I debug the
code, the value is ! itself in cluster. May be need to add more combination of
tests.
They are both same right now. But what if somebody changes them in future? The
variables/enums used are different. FlowActivityRowKeyConverter#decode(byte[])
uses Separator#QUALIFIERS whereas FlowActivityRowKeyConverter#decode(String)
uses the constants defined in TimelineReaderUtils. Same goes for encode methods.
If somebody in future changes either one of them i.e. changes QUALIFIERS but
not SEPARATOR_CHAR to something other than "!", the intention of test case
won't be served. So it would be better to use the respective constant/enum as
per the converter method being invoked. This was the intention behind comment.
bq. IIUC, public static final constants need not to be annotated
Sorry, my bad. We do use FROM_ID outside the scope of tests and
TimelineReaderUtils.
bq. RowKeyConvertor interface extended from KeyConvertor interface. RowKey is
interface which is implemented by HBase table row key class . Both
RowKeyConvertor and RowKey interfaces are different.
What I meant was to define RowKeyConverter interface as under. We do not expect
RowKeyConverter to be extended for anything other than type parameter being
subtype of RowKey. Right?
{code}
public interface RowKeyConverter extends KeyConverter
{code}
bq. Even I got doubt while removing final, why it should be final? utils class
should be public
utils classes are not expected to be subclassed. Right? They just encapsulate a
bunch of static methods and constants.
was (Author: varun_saxena):
[~rohithsharma],
bq. both are same right? any issues will happen because of enum? I debug the
code, the value is ! itself in cluster. May be need to add more combination of
tests.
They are both same right now. But what if somebody changes them in future? The
variables/enums used are different. FlowActivityRowKeyConverter#decode(byte[])
uses Separator#QUALIFIERS whereas FlowActivityRowKeyConverter#decode(String)
uses the constants defined in TimelineReaderUtils. Same goes for encode methods.
If somebody in future changes either one of them i.e. changes either of
QUALIFIERS or SEPARATOR_CHAR to something other than "!", the intention of test
case won't be served. So it would be better to use the respective constant/enum
as per the converter method being invoked. This was the intention behind
comment.
bq. IIUC, public static final constants need not to be annotated
Sorry, my bad. We do use FROM_ID outside the scope of tests and
TimelineReaderUtils.
bq. RowKeyConvertor interface extended from KeyConvertor interface. RowKey is
interface which is implemented by HBase table row key class . Both
RowKeyConvertor and RowKey interfaces are different.
What I mean was to define RowKeyConverter interface as under. We do not expect
RowKeyConverter to be extended for anything other than type parameter being
subtype of RowKey. Right?
{code}
public interface RowKeyConverter extends KeyConverter
{code}
bq. Even I got doubt while removing final, why it should be final? utils class
should be public
utils classes are not expected to be subclassed. Right? They just encapsulate a
bunch of static methods and constants.
> 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-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
[
https://issues.apache.org/jira/browse/YARN-6027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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
[jira] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
[ https://issues.apache.org/jira/browse/YARN-6027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15872457#comment-15872457 ] Varun Saxena edited comment on YARN-6027 at 2/17/17 8:19 PM: - By the way can we refactor the Timeline UID converter class to accomodate encoding/decoding of fromId as per the offline discussion we had? However, if we keep it at a storage layer, we can also have a separate class(but modeled along the lines of uid converter class) so that we do not mix up potentially unrelated areas. That is one major comment on the patch. Let me know your thoughts on it. We can have a detailed review based on that. In the current patch, we are splitting fromId in web services and also passing it to storage layer, which seems a little weird. Also I noticed that the message while throwing BadRequestException is "Invalid fromid has provided". We can say "Invalid fromid has been provided" or "Invalid fromid in request" was (Author: varun_saxena): By the way can we refactor the Timeline UID converter class to accomodate encoding/decoding of fromId as per the offline discussion we had? However, if we keep it at a storage layer, we can also have a separate class(but modeled along the lines of uid converter class) so that we do not mix up potentially unrelated areas. I think that is one major comment on the patch. Let me know your thoughts on it. We can have a detailed review based on that. In the current patch, we are splitting fromId in web services and also passing it to storage layer, which seems a little weird. Also I noticed that the message while throwing BadRequestException is "Invalid fromid has provided". We can say "Invalid fromid has been provided" or "Invalid fromid in request" > 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 > > > 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-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
[ https://issues.apache.org/jira/browse/YARN-6027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15871560#comment-15871560 ] Varun Saxena edited comment on YARN-6027 at 2/17/17 9:53 AM: - [~rohithsharma], we had discussed this in yesterday's call. I think intention was to fit this code into TimelineUIDConverter, not necessarily to return Flow UID per say (we can always add another enum). This is because functionality for encoding and decoding of escape characters and separators is in built in that class. So nobody was against adding a new field FROM_ID if FLOW_UID cannot serve our use case. We can probably modify TimelineUIDConverter to suit our use case if we want as it is an internal class. Maybe name it TimelineIDConverter or something else and add another enum for FROM_ID. We can possibly pass an ID context which passes more things than just the TimelineReaderContext. We can also use a builder pattern for it to help easy addition of fields. Havent thought it through but these are few possible options. Or you may have other suggestions. was (Author: varun_saxena): [~rohithsharma], we had discussed this in yesterday's call. I think intention was to fit this code into TimelineUIDConverter, not necessarily to return Flow UID per say (we can always add another enum). This is because functionality for encoding and decoding of escape characters and separators is in built in that class. So nobody was against adding a new field FROM_ID if FLOW_UID cannot serve our use case. We can probably modify TimelineUIDConverter to suit our use case if we want as it is an internal class. Maybe name it TimelineIDConverter or something else. We can possibly pass an ID context which passes more things than just the TimelineReaderContext. We can also use a builder pattern for it to help easy addition of fields. Havent thought it through but these are few possible options. Or you may have other suggestions. > 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: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
[
https://issues.apache.org/jira/browse/YARN-6027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (YARN-6027) Support fromid(offset) filter for /flows API
[
https://issues.apache.org/jira/browse/YARN-6027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15864389#comment-15864389
]
Varun Saxena edited comment on YARN-6027 at 2/13/17 8:46 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. "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 is incorrect. "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: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
