> On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java, > > line 122 > > <https://reviews.apache.org/r/33468/diff/1/?file=940254#file940254line122> > > > > This is useful and will avoid annonying IDE warnings, but is better > > handled in a seperate jira.
Undoing is more difficult as IDE doesn't show a warn sign, kindly overlook this time. Will raise a separate JIRA and fix it for once and all. > On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java, > > line 182 > > <https://reviews.apache.org/r/33468/diff/1/?file=940255#file940255line182> > > > > 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. Changed nominalTime to instanceTime everywhere. > On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > client/src/main/java/org/apache/falcon/resource/SchedulableEntityInstance.java, > > line 53 > > <https://reviews.apache.org/r/33468/diff/1/?file=940245#file940245line53> > > > > instancetime -> camel case > > instancetime - Null value possible ? Fixed the case and handled the null > On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/EntityUtil.java, line 789 > > <https://reviews.apache.org/r/33468/diff/1/?file=940246#file940246line789> > > > > An example input & output will be very helpful Added examples in the documentation. > On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/EntityUtil.java, line 868 > > <https://reviews.apache.org/r/33468/diff/1/?file=940246#file940246line868> > > > > 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. Incorporated the comments. > On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 538 > > <https://reviews.apache.org/r/33468/diff/1/?file=940247#file940247line538> > > > > Is it possible for outputInstance to be null here ? Shouldn't be. This method is private and is called only in scenarios where it can not be null. > On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > client/src/main/java/org/apache/falcon/resource/SchedulableEntityInstance.java, > > line 43 > > <https://reviews.apache.org/r/33468/diff/1/?file=940245#file940245line43> > > > > 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. Renamed tags to tag. > On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 546 > > <https://reviews.apache.org/r/33468/diff/1/?file=940247#file940247line546> > > > > 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) Added tests for all. No issues found. > On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 685 > > <https://reviews.apache.org/r/33468/diff/1/?file=940247#file940247line685> > > > > Like in the case of producerInstance, would like more tests for > > ConsumerInstance as well. Added the recommended tests. > On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > client/src/main/java/org/apache/falcon/resource/SchedulableEntityInstance.java, > > line 119 > > <https://reviews.apache.org/r/33468/diff/1/?file=940245#file940245line119> > > > > Potential NPE. Fixed it. > On May 8, 2015, 7:04 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 627 > > <https://reviews.apache.org/r/33468/diff/1/?file=940247#file940247line627> > > > > Potential functional correctness when input feeds have variable time > > range (for ex: today(0,0) - now(0)) Thanks for catching it. I have fixed it and added relevant unit tests also. - Ajay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33468/#review82798 ----------------------------------------------------------- On June 3, 2015, 1:32 a.m., Ajay Yadava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33468/ > ----------------------------------------------------------- > > (Updated June 3, 2015, 1:32 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 > >
