falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ajay Yadava" <ajayn...@gmail.com>
Subject Re: Review Request 33468: FALCON-1039 Instance Dependency API
Date Wed, 03 Jun 2015 01:32:18 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message