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