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, 27 Sep 2015 10:41:23 GMT


> On Sept. 22, 2015, 11:46 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 396
> > <https://reviews.apache.org/r/38450/diff/2/?file=1077636#file1077636line396>
> >
> >     My understanding was that there are multiple policies per stage. So, is this
method intended to return all policies across stages for a feed? In this case, it might useful
to return a Map<Stage, String>. If not, should Stage be one of the input params?

It's not required as of now as it's not being used for that currently.


> On Sept. 22, 2015, 11:46 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/lifecycle/retention/AgeBasedDelete.java,
line 41
> > <https://reviews.apache.org/r/38450/diff/2/?file=1077642#file1077642line41>
> >
> >     Nit : policy.retention.agebaseddelete.limit seems more representative of the
hierarchy.

this seems more natural to me as a user - retention policy agebaseddelete's limit :)


> On Sept. 22, 2015, 11:46 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/service/LifecyclePolicyMap.java, line 61
> > <https://reviews.apache.org/r/38450/diff/2/?file=1077644#file1077644line61>
> >
> >     Stage will be empty always.

Good catch. Fixed it.


> On Sept. 22, 2015, 11:46 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/service/LifecyclePolicyMap.java, line 58
> > <https://reviews.apache.org/r/38450/diff/2/?file=1077644#file1077644line58>
> >
> >     Feel adding the stage in the properties makes this user-friendly. For example,
> >     falcon.feed.lifecycle.retention.policies or falcon.feed.lifecycle.replication.policies.

It might read better but it won't be user friendly. Instead of one property there will be
many properties and we will need to add validations that the properties in each are belonging
to correct stage. Also, with every new stage added to lifecycle this code will need to be
changed.


> On Sept. 22, 2015, 11:46 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/workflow/WorkflowEngineFactory.java, line
34
> > <https://reviews.apache.org/r/38450/diff/2/?file=1077645#file1077645line34>
> >
> >     Lifecycle is not really a Workflow Engine. Rather, a workflow engine should
understand Lifecycle like it understands Entity (Feed and Process) now. This makes it difficult
to:
> >     1. Plugging in other workflow engines (such as our native scheduler)
> >     2. This is meant to be extensible by user. We don't users to be building co-ords,
bundles. It can be more like, the builder returns the user-workflow to execute (and properties).

Since we are discussing it in JIRA, I am dropping it from here, will reply on JIRA to avoid
duplicate effort :)


> On Sept. 22, 2015, 11:46 a.m., Pallavi Rao wrote:
> > common/src/test/java/org/apache/falcon/entity/parser/FeedEntityParserTest.java,
line 178
> > <https://reviews.apache.org/r/38450/diff/2/?file=1077649#file1077649line178>
> >
> >     Additional test cases can be added to check for validation against sla and late
cut off.. Not necessarily in this class.

I have already added them in AgeBasedDeleteTest, looks like you missed it :)


> On Sept. 22, 2015, 11:46 a.m., Pallavi Rao wrote:
> > client/src/main/resources/feed-0.1.xsd, line 128
> > <https://reviews.apache.org/r/38450/diff/2/?file=1077635#file1077635line128>
> >
> >     Since we are not deprecating the old retention element. Do we throw an error
if both are defined? Or are we just saying lifecycle takes precedence? Either way this needs
to be called out.

Documented the behavior.


> On Sept. 22, 2015, 11:46 a.m., Pallavi Rao wrote:
> > common/src/main/resources/startup.properties, line 48
> > <https://reviews.apache.org/r/38450/diff/2/?file=1077646#file1077646line48>
> >
> >     Can't we have a getBuilder() as method in LifeCyclePolicy and have the method
return an appropriate builder, rather than user having to specify it here.

I prefer it this way as this is a sort of poor man's DI. Moreover, it is not a big deal as
a user will need to rarely specify it.


- Ajay


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


On Sept. 27, 2015, 10:39 a.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38450/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2015, 10:39 a.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 4f5599e

>   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 e9946c4 
>   common/src/test/java/org/apache/falcon/entity/FeedHelperTest.java c70cfcc 
>   common/src/test/java/org/apache/falcon/entity/parser/FeedEntityParserTest.java b6fdb13

>   common/src/test/resources/config/feed/feed-0.3.xml PRE-CREATION 
>   common/src/test/resources/config/feed/feed-0.4.xml PRE-CREATION 
>   docs/src/site/twiki/EntitySpecification.twiki d4f4140 
>   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