You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Peeyush Bishnoi <bp...@yahoo.co.in> on 2015/12/21 10:10:41 UTC
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/
-----------------------------------------------------------
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
common/src/main/java/org/apache/falcon/metadata/InstanceRelationshipGraphBuilder.java b709857
common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java f206ff1
common/src/test/java/org/apache/falcon/metadata/MetadataMappingServiceTest.java 29f933d
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
>
>
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
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 Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/#review113285
-----------------------------------------------------------
Ship it!
Ship It!
- Ajay Yadava
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
>
>
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
> On Jan. 7, 2016, 9:20 p.m., Balu Vellanki wrote:
> >
@Balu, Thanks a lot for reviewing this.
> On Jan. 7, 2016, 9:20 p.m., Balu Vellanki wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java, line 162
> > <https://reviews.apache.org/r/41601/diff/6/?file=1186373#file1186373line162>
> >
> > This line is repeated and can be outside if-else block
Fixed.
> On Jan. 7, 2016, 9:20 p.m., Balu Vellanki wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java, line 249
> > <https://reviews.apache.org/r/41601/diff/6/?file=1186373#file1186373line249>
> >
> > Please use StringUtils.isNotBlank()
I could have used the StringUtils.isBlank but I have tried to align with other implementation as done earlier for validation.
Fixed.
> On Jan. 7, 2016, 9:20 p.m., Balu Vellanki wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java, line 253
> > <https://reviews.apache.org/r/41601/diff/6/?file=1186373#file1186373line253>
> >
> > It is better to use StringUtils.isNotBlank() method instead.
I could have used the StringUtils.isBlank but I have tried to align with other implementation as done earlier for validation.
Fixed.
> On Jan. 7, 2016, 9:20 p.m., Balu Vellanki wrote:
> > client/src/main/java/org/apache/falcon/client/FalconClient.java, line 242
> > <https://reviews.apache.org/r/41601/diff/6/?file=1186374#file1186374line242>
> >
> > If you are listing replication metrics, then this operation is not necessary. You should handle it as part of the LIST operation.
fixed.
> On Jan. 7, 2016, 9:20 p.m., Balu Vellanki wrote:
> > prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java, line 108
> > <https://reviews.apache.org/r/41601/diff/6/?file=1186377#file1186377line108>
> >
> > replication-metrics is the {type} in listDimensionValues method above, it is not needed to write a separate method here.
If you have followed up the discussion with Ajay, earlier I have done impementation to handle replication-metrics in single method only. But for that single method there will be a long list of parameters, many of which is not required by other API's. And replication-metrics is not for all the entities but specifically to list the replication metrics from the repliction recipe's and feed replication. That's why I have used the @Path("replication-metrics/list") .
As I have mentioned in my earlier comment that we have to think to refactor sendMetadataDiscoveryRequest method to separate Metadiscovery List and Relations and then we can accomodate replication-metrics in Metadiscovery List appropriately. I will create the JIRA issue for refactoring the sendMetadataDiscoveryRequest method.
- Peeyush
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/#review113323
-----------------------------------------------------------
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
>
>
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Balu Vellanki <bv...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/#review113323
-----------------------------------------------------------
client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java (line 162)
<https://reviews.apache.org/r/41601/#comment173862>
This line is repeated and can be outside if-else block
client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java (line 249)
<https://reviews.apache.org/r/41601/#comment173865>
Please use StringUtils.isNotBlank()
client/src/main/java/org/apache/falcon/cli/FalconMetadataCLI.java (line 253)
<https://reviews.apache.org/r/41601/#comment173864>
It is better to use StringUtils.isNotBlank() method instead.
client/src/main/java/org/apache/falcon/client/FalconClient.java (line 242)
<https://reviews.apache.org/r/41601/#comment173860>
If you are listing replication metrics, then this operation is not necessary. You should handle it as part of the LIST operation.
prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java (line 108)
<https://reviews.apache.org/r/41601/#comment173866>
replication-metrics is the {type} in listDimensionValues method above, it is not needed to write a separate method here.
- Balu Vellanki
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
>
>
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
> On Jan. 9, 2016, 5:21 a.m., Ying Zheng wrote:
> > client/src/main/java/org/apache/falcon/client/FalconClient.java, line 1043
> > <https://reviews.apache.org/r/41601/diff/7/?file=1187525#file1187525line1043>
> >
> > nitpick: better to use "isNotEmpty" for simplicity and consistency.
fixed.
> On Jan. 9, 2016, 5:21 a.m., Ying Zheng wrote:
> > prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java, line 121
> > <https://reviews.apache.org/r/41601/diff/7/?file=1187528#file1187528line121>
> >
> > From your code, you only return results when the user specify an entity name. Is this expected? If so, it looks better to make the entity name as a required parameter.
fixed.
> On Jan. 9, 2016, 5:21 a.m., Ying Zheng wrote:
> > prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java, line 138
> > <https://reviews.apache.org/r/41601/diff/7/?file=1187528#file1187528line138>
> >
> > Should be vertex for feed and process instead of cluster? Also, feed and process could share the same name, so you need both type and name to uniquely determine an entity.
fixed.
- Peeyush
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/#review113567
-----------------------------------------------------------
On Jan. 8, 2016, 3:52 p.m., Peeyush Bishnoi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41601/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2016, 3:52 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
>
>
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Ying Zheng <yz...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/#review113567
-----------------------------------------------------------
client/src/main/java/org/apache/falcon/client/FalconClient.java (line 1043)
<https://reviews.apache.org/r/41601/#comment174300>
nitpick: better to use "isNotEmpty" for simplicity and consistency.
prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java (line 121)
<https://reviews.apache.org/r/41601/#comment174308>
From your code, you only return results when the user specify an entity name. Is this expected? If so, it looks better to make the entity name as a required parameter.
prism/src/main/java/org/apache/falcon/resource/metadata/MetadataDiscoveryResource.java (line 138)
<https://reviews.apache.org/r/41601/#comment174366>
Should be vertex for feed and process instead of cluster? Also, feed and process could share the same name, so you need both type and name to uniquely determine an entity.
- Ying Zheng
On Jan. 8, 2016, 3:52 p.m., Peeyush Bishnoi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41601/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2016, 3:52 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
>
>
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/
-----------------------------------------------------------
(Updated Jan. 10, 2016, 9:56 a.m.)
Review request for Falcon.
Changes
-------
Updated patch after handling review comments.
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 (updated)
-----
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 2188ba4
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 0f7701c
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 Venkat Ranganathan <n....@live.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/#review113500
-----------------------------------------------------------
Ship it!
LGTM. Thanks for incorporating the review comments.
- Venkat Ranganathan
On Jan. 8, 2016, 7:52 a.m., Peeyush Bishnoi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41601/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2016, 7:52 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/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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/
-----------------------------------------------------------
(Updated Jan. 8, 2016, 3:52 p.m.)
Review request for Falcon.
Changes
-------
Updated patch after incorporating review comments.
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 (updated)
-----
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
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 41601: Add CLI option to display captured
replication metrics
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/
-----------------------------------------------------------
(Updated Jan. 7, 2016, 6:11 p.m.)
Review request for Falcon.
Changes
-------
Updated patch after incorporating review comments.
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 (updated)
-----
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>.
-----------------------------------------------------------
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 (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/
-----------------------------------------------------------
(Updated Dec. 24, 2015, 7:35 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 (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41601/
-----------------------------------------------------------
(Updated Dec. 21, 2015, 9:15 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 (updated)
-----
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