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 38450: FALCON-965 Open up Feed lifecycle for user extension
Date Sun, 20 Sep 2015 16:02:06 GMT


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > lifecycle/src/main/java/org/apache/falcon/lifecycle/engine/oozie/utils/OozieBuilderUtils.java,
line 434
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075859#file1075859line434>
> >
> >     Can we move this function to class HiveUtil.

It is not useful outside of OozieBuilder, so it makes more sense to keep it here.


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > client/src/main/resources/feed-0.1.xsd, line 120
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075837#file1075837line120>
> >
> >     Can you add details about lifecycle in xsd doc section.

Details about lifecycle are already present at the position where element is defined.


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > client/src/main/resources/feed-0.1.xsd, line 150
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075837#file1075837line150>
> >
> >     As we are defining retention through lifecycle, do we require older retention
specifically. Is this required for backward compatibility, please confirm.

You are right, this is requried for backward compatibility.


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 402
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075838#file1075838line402>
> >
> >     "limit" is the key attribute for retention. It does not seem good to keep limit
in key-value property section. It should be element in retention-stage.

Limit has to be a generic type because not all policies will have same type of retention limit
e.g. size based delete and instance time based delete have two different types of limits.


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 429
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075838#file1075838line429>
> >
> >     Returning DefaultPolicyName does not make sense. If user want to use retention
stage, they must specify the required policy.

It is an optional element on lines of Convention over Configuration.


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > common/src/main/java/org/apache/falcon/entity/parser/FeedEntityParser.java, line
128
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075840#file1075840line128>
> >
> >     What is the requirement to define two type of retention in entity global and
cluster. From user experience perspective, how this will help the user. Can we think to make
it as one like currently.

Making it one will not solve cases where we need different retention behavior in different
clusters. It is an improvement over the existing behavior. Other feeds like validity, sla
etc. can also be overridden at cluster level.


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > lifecycle/src/main/java/org/apache/falcon/lifecycle/engine/oozie/utils/OozieBuilderUtils.java,
line 393
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075859#file1075859line393>
> >
> >     Can we move this function to class HiveUtil.

It is not useful outside of OozieBuilder, so it makes more sense to keep it here.


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > lifecycle/src/main/java/org/apache/falcon/lifecycle/engine/oozie/utils/OozieBuilderUtils.java,
line 359
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075859#file1075859line359>
> >
> >     Can we move this function to class HiveUtil.

It is not useful outside of OozieBuilder, so it makes more sense to keep it here.


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > lifecycle/src/main/java/org/apache/falcon/lifecycle/engine/oozie/utils/OozieBuilderUtils.java,
line 301
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075859#file1075859line301>
> >
> >     What about support for other action like FS,SSH,Sqoop .

How are actions like ssh and sqoop relevant in retention?


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > lifecycle/src/main/java/org/apache/falcon/lifecycle/engine/oozie/retention/NominalTimeBasedWorkflowBuilder.java,
line 119
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075858#file1075858line119>
> >
> >     Can we move this function to class HiveUtil.

It is "workflow" specific and Hive doesn't understand workflow, hence it is better to be put
here.


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > common/src/main/java/org/apache/falcon/lifecycle/retention/NominalTimeBasedDelete.java,
line 60
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075845#file1075845line60>
> >
> >     "limit" is the key attribute for retention. It does not seem good to keep limit
in key-value property section. It should be element in retention-stage.

Limit has to be a generic type because not all policies will have same type of retention limit
e.g. size based delete and instance time based delete have two different types of limits.


> On Sept. 18, 2015, 10:35 a.m., Peeyush Bishnoi wrote:
> > src/conf/startup.properties, line 55
> > <https://reviews.apache.org/r/38450/diff/1/?file=1075868#file1075868line55>
> >
> >     Can we make the lifecycle policy to be defined like: *.falcon.feed.lifecycle.policies=org.apache.falcon.lifecycle.retention.NominalTimeBasedDelete#NominalTimeBasedDelete

This seems to be a break from current convention. What benefits this has over current scheme?


- Ajay


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38450/#review99507
-----------------------------------------------------------


On Sept. 20, 2015, 3:40 p.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38450/
> -----------------------------------------------------------
> 
> (Updated Sept. 20, 2015, 3:40 p.m.)
> 
> 
> Review request for Falcon.
> 
> 
> Bugs: FALCON-965
>     https://issues.apache.org/jira/browse/FALCON-965
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> For details on feed lifecycle feature and motivation behind it, please refer FALCON-965.
This is the base framework for lifecycle with a feature parity of retention stage as a reference
implementation.
> 
> 
> Diffs
> -----
> 
>   client/src/main/resources/feed-0.1.xsd 2af28d2 
>   common/src/main/java/org/apache/falcon/entity/FeedHelper.java 572923b 
>   common/src/main/java/org/apache/falcon/entity/parser/FeedEntityParser.java 992fc51

>   common/src/main/java/org/apache/falcon/lifecycle/AbstractPolicyBuilderFactory.java
PRE-CREATION 
>   common/src/main/java/org/apache/falcon/lifecycle/FeedLifecycleStage.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/lifecycle/LifecyclePolicy.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/lifecycle/PolicyBuilder.java PRE-CREATION 
>   common/src/main/java/org/apache/falcon/lifecycle/retention/AgeBasedDelete.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/lifecycle/retention/RetentionPolicy.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/service/LifecyclePolicyMap.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/workflow/WorkflowEngineFactory.java 756c6b8

>   common/src/main/resources/startup.properties 9db460c 
>   common/src/test/java/org/apache/falcon/entity/AbstractTestBase.java 6179855 
>   common/src/test/java/org/apache/falcon/entity/FeedHelperTest.java c70cfcc 
>   common/src/test/java/org/apache/falcon/entity/parser/FeedEntityParserTest.java 1e9b72f

>   common/src/test/resources/config/feed/feed-0.3.xml PRE-CREATION 
>   common/src/test/resources/config/feed/feed-0.4.xml PRE-CREATION 
>   lifecycle/pom.xml PRE-CREATION 
>   lifecycle/src/main/java/org/apache/falcon/lifecycle/engine/oozie/OoziePolicyBuilderFactory.java
PRE-CREATION 
>   lifecycle/src/main/java/org/apache/falcon/lifecycle/engine/oozie/retention/AgeBasedCoordinatorBuilder.java
PRE-CREATION 
>   lifecycle/src/main/java/org/apache/falcon/lifecycle/engine/oozie/retention/AgeBasedDeleteBuilder.java
PRE-CREATION 
>   lifecycle/src/main/java/org/apache/falcon/lifecycle/engine/oozie/retention/AgeBasedWorkflowBuilder.java
PRE-CREATION 
>   lifecycle/src/main/java/org/apache/falcon/lifecycle/engine/oozie/utils/OozieBuilderUtils.java
PRE-CREATION 
>   lifecycle/src/main/resources/action/feed/eviction-action.xml PRE-CREATION 
>   lifecycle/src/main/resources/binding/jaxb-binding.xjb PRE-CREATION 
>   lifecycle/src/test/java/org/apache/falcon/lifecycle/retention/AgeBasedDeleteTest.java
PRE-CREATION 
>   oozie/pom.xml 157edf9 
>   oozie/src/main/java/org/apache/falcon/oozie/feed/FeedBundleBuilder.java b819dee 
>   oozie/src/test/java/org/apache/falcon/oozie/feed/OozieFeedWorkflowBuilderTest.java
7d0174a 
>   oozie/src/test/resources/feed/fs-retention-feed.xml PRE-CREATION 
>   oozie/src/test/resources/feed/fs-retention-lifecycle-feed.xml PRE-CREATION 
>   pom.xml 646de69 
>   src/conf/startup.properties 8f3bc35 
> 
> Diff: https://reviews.apache.org/r/38450/diff/
> 
> 
> Testing
> -------
> 
> Unit tests have been added for all the methods.
> I have compared the new artifacts(workflow, bundle, config-default and coordinator xmls)
with the old ones.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>


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