falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Srikanth Sundarrajan" <srik...@hotmail.com>
Subject Re: Review Request 33468: FALCON-1039 Instance Dependency API
Date Fri, 08 May 2015 07:04:16 GMT

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


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