----------------------------------------------------------- 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 > >
