----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33468/#review82798 -----------------------------------------------------------
client/src/main/java/org/apache/falcon/cli/FalconCLI.java <https://reviews.apache.org/r/33468/#comment133858> Would prefer that we avoid using the term nominalTime, instead can we call this instanceTime to be more generic and less oozieish? client/src/main/java/org/apache/falcon/resource/EntityList.java <https://reviews.apache.org/r/33468/#comment133859> Are these duplicates from ScheduleableEntityInstance ? Any possibility of avoiding it, if they are indeed duplicates ? client/src/main/java/org/apache/falcon/resource/SchedulableEntityInstance.java <https://reviews.apache.org/r/33468/#comment133619> Would it be better if this was an enum {INPUT, OUTPUT} ? Also from the name it seemed like a collection of tags, but seems to be holding a single string. client/src/main/java/org/apache/falcon/resource/SchedulableEntityInstance.java <https://reviews.apache.org/r/33468/#comment133621> instancetime -> camel case instancetime - Null value possible ? client/src/main/java/org/apache/falcon/resource/SchedulableEntityInstance.java <https://reviews.apache.org/r/33468/#comment133620> Potential NPE. common/src/main/java/org/apache/falcon/entity/EntityUtil.java <https://reviews.apache.org/r/33468/#comment133622> empty line common/src/main/java/org/apache/falcon/entity/EntityUtil.java <https://reviews.apache.org/r/33468/#comment133623> An example input & output will be very helpful common/src/main/java/org/apache/falcon/entity/EntityUtil.java <https://reviews.apache.org/r/33468/#comment133857> Would 1ms increment be sufficient ? If so, it would be more intuitive to use ONE_MS constant here. 59seconds might mislead reader into thinking about its significance. common/src/main/java/org/apache/falcon/entity/FeedHelper.java <https://reviews.apache.org/r/33468/#comment133860> Is it possible for outputInstance to be null here ? common/src/main/java/org/apache/falcon/entity/FeedHelper.java <https://reviews.apache.org/r/33468/#comment133861> This is very core to the functioning of this dependencies api. Would be great if we add more tests in FeedHelper for this. Some sample scenarios of interest: * instance=now(0) * instance=yesterday(0,0) * instance=now(4) * instance=now(-30) common/src/main/java/org/apache/falcon/entity/FeedHelper.java <https://reviews.apache.org/r/33468/#comment133863> Potential functional correctness when input feeds have variable time range (for ex: today(0,0) - now(0)) common/src/main/java/org/apache/falcon/entity/FeedHelper.java <https://reviews.apache.org/r/33468/#comment133862> Like in the case of producerInstance, would like more tests for ConsumerInstance as well. docs/src/site/twiki/FalconCLI.twiki <https://reviews.apache.org/r/33468/#comment133864> Can we drop the nominal time in the args please? Some sample outputs will be very useful Are there any gotchas with this api ? How about latest() or other similar time boundaries which are non-deterministic. Can we call them out prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java <https://reviews.apache.org/r/33468/#comment133617> This is useful and will avoid annonying IDE warnings, but is better handled in a seperate jira. prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java <https://reviews.apache.org/r/33468/#comment133618> Sorry for the nitpick. Nominal time is a very oozie specific term and it would be better to rename this to instance time. Corresponding changes to CLI will be also be desirable. - Srikanth Sundarrajan On April 23, 2015, 5:55 a.m., Ajay Yadava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33468/ > ----------------------------------------------------------- > > (Updated April 23, 2015, 5:55 a.m.) > > > Review request for Falcon and Srikanth Sundarrajan. > > > Bugs: FALCON-1039 > https://issues.apache.org/jira/browse/FALCON-1039 > > > Repository: falcon-git > > > Description > ------- > > API to list instances which are dependent on the given instance or which this > instance is dependent on. For more details refer the JIRA > > > Diffs > ----- > > client/src/main/java/org/apache/falcon/ResponseHelper.java 2261ceb > client/src/main/java/org/apache/falcon/cli/FalconCLI.java 7d56b01 > client/src/main/java/org/apache/falcon/client/FalconClient.java fedcea6 > client/src/main/java/org/apache/falcon/resource/EntityList.java 4c96195 > > client/src/main/java/org/apache/falcon/resource/InstanceDependencyResult.java > PRE-CREATION > > client/src/main/java/org/apache/falcon/resource/SchedulableEntityInstance.java > PRE-CREATION > common/src/main/java/org/apache/falcon/entity/EntityUtil.java 26d3da2 > common/src/main/java/org/apache/falcon/entity/FeedHelper.java 20f348d > common/src/main/java/org/apache/falcon/entity/ProcessHelper.java 29aefa0 > common/src/test/java/org/apache/falcon/entity/FeedHelperTest.java 63ab7da > common/src/test/java/org/apache/falcon/entity/ProcessHelperTest.java > PRE-CREATION > docs/src/site/twiki/FalconCLI.twiki 0e42ae2 > docs/src/site/twiki/restapi/InstanceDependency.twiki PRE-CREATION > docs/src/site/twiki/restapi/ResourceList.twiki 060e0af > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java > 25cb312 > prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java > f0c4596 > > prism/src/main/java/org/apache/falcon/resource/proxy/InstanceManagerProxy.java > e304bd8 > webapp/src/main/java/org/apache/falcon/resource/InstanceManager.java > dc533a2 > > webapp/src/main/java/org/apache/falcon/resource/SchedulableEntityManager.java > 82a622c > webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java dd14e9c > > Diff: https://reviews.apache.org/r/33468/diff/ > > > Testing > ------- > > Unit tests and Integration tests have been added for the feature. > Have also tested in embedded and distributed mode manually. > > > Thanks, > > Ajay Yadava > >
