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