You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Ajay Yadava <aj...@gmail.com> on 2016/01/05 06:12:27 UTC
Re: Review Request 41601: Add CLI option to display captured
replication metrics
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/#review112763
-----------------------------------------------------------
client/src/main/java/org/apache/falcon/cli/FalconCLI.java (line 64)
<https://reviews.apache.org/r/41601/#comment173252>
Shouldn't this be just one ENTITY_TYPE_OPT with value being feed/process?
client/src/main/java/org/apache/falcon/cli/FalconCLI.java (line 87)
<https://reviews.apache.org/r/41601/#comment173251>
can we use the same parameter for specifying limit as in other api? I believe it's called numResults
client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java (line 148)
<https://reviews.apache.org/r/41601/#comment173253>
Limit value shouldn't be affected by value of other fields. It should be what is specified/default value. I can understand that this might be a shortcut for validations but it's better to keep them separate.
client/src/main/java/org/apache/falcon/client/FalconClient.java (line 626)
<https://reviews.apache.org/r/41601/#comment173254>
Please create another method instead of using a common method like sendMetadataDiscoveryRequest. Disadvantage in this approach is that with every parameter or new api (e.g. in this case schedEntityType) you will end up affecting other api calls and it will be a long list of parameters.
client/src/main/java/org/apache/falcon/metadata/RelationshipType.java (line 43)
<https://reviews.apache.org/r/41601/#comment173255>
metrics is very generic and doesn't convey the purpose of the api. Can we please make it more specific like replication-metrics or something?
docs/src/site/twiki/FalconCLI.twiki (line 482)
<https://reviews.apache.org/r/41601/#comment173256>
There should be entity type, entity name and the no argument option -replicationMetrics. This will make the call more consistent with other api calls as type everywhere else is used for entity type.
prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java (line 75)
<https://reviews.apache.org/r/41601/#comment173262>
It will be really useful to have a time based filter (start, end) based on the instance time.
prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java (line 77)
<https://reviews.apache.org/r/41601/#comment173257>
Default value should be taken from default number of results which is another common patter in all other paginated api calls.
prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java (line 78)
<https://reviews.apache.org/r/41601/#comment173261>
I assume default value of -1 will mean return all results that is not very helpful as then by default it will fetch a very large number of results for a long running replication feed. Most of the time people won't need so many results, so it will be better to have default more tuned towards most common use case.
prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java (line 198)
<https://reviews.apache.org/r/41601/#comment173259>
nit: It will be very useful to document the relevant relationships and how to query in plain english here.
- Ajay Yadava
On Dec. 24, 2015, 7:58 a.m., Peeyush Bishnoi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41601/
> -----------------------------------------------------------
>
> (Updated Dec. 24, 2015, 7:58 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1643
> https://issues.apache.org/jira/browse/FALCON-1643
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> FALCON-1643 : Add CLI option to display captured replication metrics
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/falcon/cli/FalconCLI.java 24f230a
> client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java 36dd613
> client/src/main/java/org/apache/falcon/client/FalconClient.java aea39a6
> client/src/main/java/org/apache/falcon/metadata/RelationshipType.java 8e5f8ea
> docs/src/site/twiki/FalconCLI.twiki 26e6b33
> prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java 60c1089
> prism/src/test/java/org/apache/falcon/resource/metadata/LineageMetadataResourceTest.java fb4ff6f
> prism/src/test/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResourceTest.java 84ada9a
> prism/src/test/java/org/apache/falcon/resource/metadata/MetadataTestContext.java 05cc2e9
>
> Diff: https://reviews.apache.org/r/41601/diff/
>
>
> Testing
> -------
>
> Unit test cased added. Yes.
>
>
> Thanks,
>
> Peeyush Bishnoi
>
>
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconCLI.java, line 64
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176051#file1176051line64>
> >
> > Shouldn't this be just one ENTITY_TYPE_OPT with value being feed/process?
>From the CLI user will provide whether provided entity name is of type feed or process and internally assigned both these schedulable entities to one variable "schedEntityType".
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconCLI.java, line 87
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176051#file1176051line87>
> >
> > can we use the same parameter for specifying limit as in other api? I believe it's called numResults
Fixed.
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java, line 75
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176056#file1176056line75>
> >
> > It will be really useful to have a time based filter (start, end) based on the instance time.
This is not in the current scope but will look into it to provide later. I will create a JIRA issue for this.
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/client/FalconClient.java, line 626
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176053#file1176053line626>
> >
> > Please create another method instead of using a common method like sendMetadataDiscoveryRequest. Disadvantage in this approach is that with every parameter or new api (e.g. in this case schedEntityType) you will end up affecting other api calls and it will be a long list of parameters.
I have not created another method as I am of the opinion to align with existing metadiscovry List API. And I don't think so that it will impact other API call's. Also sendMetadataDiscoveryRequest API should be refactorred in the Metadiscovery List and Relations API separately.
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > docs/src/site/twiki/FalconCLI.twiki, line 482
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176055#file1176055line482>
> >
> > There should be entity type, entity name and the no argument option -replicationMetrics. This will make the call more consistent with other api calls as type everywhere else is used for entity type.
Metadiscovery list api supports following type: [cluster_entity|feed_entity|process_entity|user|colo|tags|groups|pipelines], so I have extend this by adding new type. I will rename to "replication_metrics".
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/metadata/RelationshipType.java, line 43
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176054#file1176054line43>
> >
> > metrics is very generic and doesn't convey the purpose of the api. Can we please make it more specific like replication-metrics or something?
Fixed.
- Peeyush
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/#review112763
-----------------------------------------------------------
On Dec. 24, 2015, 7:58 a.m., Peeyush Bishnoi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41601/
> -----------------------------------------------------------
>
> (Updated Dec. 24, 2015, 7:58 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1643
> https://issues.apache.org/jira/browse/FALCON-1643
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> FALCON-1643 : Add CLI option to display captured replication metrics
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/falcon/cli/FalconCLI.java 24f230a
> client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java 36dd613
> client/src/main/java/org/apache/falcon/client/FalconClient.java aea39a6
> client/src/main/java/org/apache/falcon/metadata/RelationshipType.java 8e5f8ea
> docs/src/site/twiki/FalconCLI.twiki 26e6b33
> prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java 60c1089
> prism/src/test/java/org/apache/falcon/resource/metadata/LineageMetadataResourceTest.java fb4ff6f
> prism/src/test/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResourceTest.java 84ada9a
> prism/src/test/java/org/apache/falcon/resource/metadata/MetadataTestContext.java 05cc2e9
>
> Diff: https://reviews.apache.org/r/41601/diff/
>
>
> Testing
> -------
>
> Unit test cased added. Yes.
>
>
> Thanks,
>
> Peeyush Bishnoi
>
>
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java, line 77
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176056#file1176056line77>
> >
> > Default value should be taken from default number of results which is another common patter in all other paginated api calls.
Fixed.
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java, line 78
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176056#file1176056line78>
> >
> > I assume default value of -1 will mean return all results that is not very helpful as then by default it will fetch a very large number of results for a long running replication feed. Most of the time people won't need so many results, so it will be better to have default more tuned towards most common use case.
I have initially thought on this and even mentioned in JIRA that by default, API will fetch the replication metrics for latest 5 instances. I think we can put the value 10, by default. If user want all the repliction metrics for entity, user can specify the value -1.
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java, line 198
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176056#file1176056line198>
> >
> > nit: It will be very useful to document the relevant relationships and how to query in plain english here.
Fixed.
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > client/src/main/java/org/apache/falcon/client/FalconClient.java, line 626
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176053#file1176053line626>
> >
> > Please create another method instead of using a common method like sendMetadataDiscoveryRequest. Disadvantage in this approach is that with every parameter or new api (e.g. in this case schedEntityType) you will end up affecting other api calls and it will be a long list of parameters.
>
> Peeyush Bishnoi wrote:
> I have not created another method as I am of the opinion to align with existing metadiscovry List API. And I don't think so that it will impact other API call's. Also sendMetadataDiscoveryRequest API should be refactorred in the Metadiscovery List and Relations API separately.
I have created another method for replication metrics. But going forward we should think to refactor sendMetadataDiscoveryRequest API to separate Metadiscovery List and Relations.
- Peeyush
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/#review112763
-----------------------------------------------------------
On Dec. 24, 2015, 7:58 a.m., Peeyush Bishnoi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41601/
> -----------------------------------------------------------
>
> (Updated Dec. 24, 2015, 7:58 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1643
> https://issues.apache.org/jira/browse/FALCON-1643
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> FALCON-1643 : Add CLI option to display captured replication metrics
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/falcon/cli/FalconCLI.java 24f230a
> client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java 36dd613
> client/src/main/java/org/apache/falcon/client/FalconClient.java aea39a6
> client/src/main/java/org/apache/falcon/metadata/RelationshipType.java 8e5f8ea
> docs/src/site/twiki/FalconCLI.twiki 26e6b33
> prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java 60c1089
> prism/src/test/java/org/apache/falcon/resource/metadata/LineageMetadataResourceTest.java fb4ff6f
> prism/src/test/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResourceTest.java 84ada9a
> prism/src/test/java/org/apache/falcon/resource/metadata/MetadataTestContext.java 05cc2e9
>
> Diff: https://reviews.apache.org/r/41601/diff/
>
>
> Testing
> -------
>
> Unit test cased added. Yes.
>
>
> Thanks,
>
> Peeyush Bishnoi
>
>
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Ajay Yadava <aj...@gmail.com>.
> On Jan. 5, 2016, 5:12 a.m., Ajay Yadava wrote:
> > prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java, line 75
> > <https://reviews.apache.org/r/41601/diff/4/?file=1176056#file1176056line75>
> >
> > It will be really useful to have a time based filter (start, end) based on the instance time.
>
> Peeyush Bishnoi wrote:
> This is not in the current scope but will look into it to provide later. I will create a JIRA issue for this.
Makes sense.
- Ajay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/#review112763
-----------------------------------------------------------
On Jan. 7, 2016, 6:23 p.m., Peeyush Bishnoi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41601/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2016, 6:23 p.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1643
> https://issues.apache.org/jira/browse/FALCON-1643
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> FALCON-1643 : Add CLI option to display captured replication metrics
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/falcon/cli/FalconCLI.java 24f230a
> client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java 36dd613
> client/src/main/java/org/apache/falcon/client/FalconClient.java aea39a6
> client/src/main/java/org/apache/falcon/metadata/RelationshipType.java 8e5f8ea
> docs/src/site/twiki/FalconCLI.twiki 26e6b33
> prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java 60c1089
> prism/src/test/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResourceTest.java 84ada9a
> prism/src/test/java/org/apache/falcon/resource/metadata/MetadataTestContext.java 05cc2e9
>
> Diff: https://reviews.apache.org/r/41601/diff/
>
>
> Testing
> -------
>
> Unit test cased added. Yes.
>
>
> Thanks,
>
> Peeyush Bishnoi
>
>