> On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > checkstyle/src/main/resources/falcon/checkstyle.xml, line 233 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104008#file1104008line233> > > > > This disables it for the entire code base. Please only disable it for > > particular lines if it is a limitation - > > http://stackoverflow.com/questions/4023185/how-to-disable-a-particular-checkstyle-rule-for-a-particular-line-of-code
It won't disable until we externally specify suspend check in java code. I have specified only in 2 places where it is unavoidable. > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > common/src/main/resources/startup.properties, line 252 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104010#file1104010line252> > > > > Should we keep this set of properties commented out and explicitly > > mentioning that these are needed only for native scheduler? Sure will do > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > pom.xml, line 116 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104011#file1104011line116> > > > > Move to 2.4, unless there is a particular dependency on this version. sure. > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java, > > line 51 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104013#file1104013line51> > > > > CreationTime is when the instance is created. If at all it is needed > > while restoring an instance from state, I would rather have a second > > constructor. will do > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 88 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104015#file1104015line88> > > > > Change all nominalTime to instanceTime. Srikanth was saying that we > > should avoid the Oozie nomenclature. will fix > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 192 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104015#file1104015line192> > > > > Nit: When is this null? The instance variable is initialized with an > > empty array list. No harm with an additional check though. Removed it > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 312 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104015#file1104015line312> > > > > Can you move this to Predicate class as a utility method? sure > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 320 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104015#file1104015line320> > > > > This has a slight problem. If the predicates are out of order in the > > list, are they not equal? I think the list should be equal irrespective of > > the order in which the elements appear. Changed awaited predicates to set and everything taken care. > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/StateService.java, line 141 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104018#file1104018line141> > > > > Nit: Use isEmpty() instead sure > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java, > > line 46 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104020#file1104020line46> > > > > Please add Javadoc comments to the methods. sure will add > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java, > > line 51 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104020#file1104020line51> > > > > Can we have an additional constructor that takes in all the mandatory > > parameters? If possible we should get rid of the default constructor all > > together. Open JPA requires empty constructor and it's not possible > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java, > > line 67 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104020#file1104020line67> > > > > Lets store the definition as clob in the state store, that will avoid > > consistency issues. As discussed offline, even though we add entity defintion , retrieving it is expensive. We will discuss about in detail later. > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java, > > line 105 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104020#file1104020line105> > > > > See if you can avoid default constructor or at least have a constructor > > with mandatory parameters and null checks there in. As i already said Open JPA wont allow this. > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java, > > line 121 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104020#file1104020line121> > > > > We are losing the timezone information here? Believe there is some > > extra handling required to preserve the timezone in the DB. > > > > Comment applies to all Timestamp instances created in this class. Looks like Timestamp class always give time w.r.t to GMT and it doesn't accept any timezone class. In dev testing in IST , this worked out. > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java, > > line 206 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104020#file1104020line206> > > > > Make the access level private It needed in test cases. > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java, > > line 216 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104020#file1104020line216> > > > > Throw a OperationNotSupported Exception. Sure will fix. > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/EntityBean.java, line > > 18 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104021#file1104021line18> > > > > Can we move all the JDBC based persistent store classes to > > org.apache.falcon.state.store.jdbc ? sure > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/EntityBean.java, line > > 33 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104021#file1104021line33> > > > > Add constraints, such as not null? For this table, in particular, none > > of the columns can be null. will add > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/EntityBean.java, line > > 43 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104021#file1104021line43> > > > > ENTITY_JOBS? May be just call it ENTITIES? will do > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/EntityBean.java, line > > 44 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104021#file1104021line44> > > > > Eventually, I would like this to replace config store. So, we should be > > storing the entity definition as a clob. Also, this will enable us to > > reconstruct state just from the state store, without having to go to config > > store. I already said this in comments > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java, > > line 116 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104023#file1104023line116> > > > > Just calling the clear() method should do it. right? Yes my bad > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/InstanceBean.java, > > line 50 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104024#file1104024line50> > > > > Use INSTANCES, rather than INSTANCE_JOBS. Sure > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/InstanceBean.java, > > line 51 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104024#file1104024line51> > > > > Should we also add some constraints such not null? will add > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/InstanceBean.java, > > line 54 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104024#file1104024line54> > > > > How are we modelling the relationship between instance and entity. > > Ideally, entity Id should be a foriegn key. Added entity id in instance table . Cascading delete and foreign key constraints will work later. I will raise jira for the same. > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/InstanceBean.java, > > line 68 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104024#file1104024line68> > > > > Remove any variable names with nominalTime. Srikanth was saying that is > > a Oozie nomenclature and that we should use instanceTime instead. sure > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java, > > line 40 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104026#file1104026line40> > > > > Call it JDBCStateStore to be clear. sure > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java, > > line 60 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104026#file1104026line60> > > > > Do we really need this check? Won't entityManager.perist() return an > > error if the row already exists? Yes we need this check as per documentation entityManager.perist() it can throw any persistence Exception. > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java, > > line 81 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104026#file1104026line81> > > > > All "queries" (get calls) are also demarcated with transaction > > boundaries, is that really required? As long as create and update are > > transactional, reads should not be dirty. Isn't it? Sure will do > On Oct. 26, 2015, 9:25 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java, > > line 122 > > <https://reviews.apache.org/r/39588/diff/1/?file=1104026#file1104026line122> > > > > Required? Wouldn't update call throw an error, if row exists? Required as i said in comments - pavan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39588/#review103982 ----------------------------------------------------------- On Oct. 23, 2015, 11:17 a.m., pavan kumar kolamuri wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39588/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2015, 11:17 a.m.) > > > Review request for Falcon. > > > Bugs: https://issues.apache.org/jira/browse/FALCON-1234 > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-1234 > > > Repository: falcon-git > > > Description > ------- > > Persistent State Store for Falcon Native Scheduler > > > Diffs > ----- > > checkstyle/src/main/resources/falcon/checkstyle.xml 2130e73 > checkstyle/src/main/resources/falcon/findbugs-exclude.xml 0a7580d > common/src/main/resources/startup.properties cc5212a > pom.xml 87c55e3 > scheduler/pom.xml 20a91d2 > scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java > 3869ff2 > > scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java > b959320 > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java > 19089c4 > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java fb4ce82 > scheduler/src/main/java/org/apache/falcon/state/ID.java 420c856 > scheduler/src/main/java/org/apache/falcon/state/StateService.java 81357a4 > > scheduler/src/main/java/org/apache/falcon/state/store/AbstractStateStore.java > 67e047f > scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/store/EntityBean.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/store/EntityStateStore.java > 4aa6fdb > > scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java > 3822860 > scheduler/src/main/java/org/apache/falcon/state/store/InstanceBean.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/state/store/InstanceStateStore.java > d6a4b49 > > scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/store/StateStore.java > f595c26 > > scheduler/src/main/java/org/apache/falcon/state/store/ValidateConnectionBean.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/state/store/service/FalconJPAService.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java > PRE-CREATION > scheduler/src/main/resources/META-INF/persistence.xml PRE-CREATION > scheduler/src/main/resources/falcon-buildinfo.properties PRE-CREATION > > scheduler/src/test/java/org/apache/falcon/execution/FalconExecutionServiceTest.java > b2f9e59 > > scheduler/src/test/java/org/apache/falcon/notification/service/SchedulerServiceTest.java > b4a0f35 > > scheduler/src/test/java/org/apache/falcon/state/AbstractSchedulerTestBase.java > PRE-CREATION > scheduler/src/test/java/org/apache/falcon/state/EntityStateServiceTest.java > 2f32b43 > > scheduler/src/test/java/org/apache/falcon/state/InstanceStateServiceTest.java > d27ac7e > > scheduler/src/test/java/org/apache/falcon/state/service/TestFalconJPAService.java > PRE-CREATION > > scheduler/src/test/java/org/apache/falcon/state/service/store/TestPersistentStateStore.java > PRE-CREATION > > scheduler/src/test/java/org/apache/falcon/tools/TestFalconStateStoreDBCLI.java > PRE-CREATION > scheduler/src/test/resources/startup.properties PRE-CREATION > src/conf/startup.properties dc9e393 > unit/src/main/resources/startup.properties fe6f430 > > Diff: https://reviews.apache.org/r/39588/diff/ > > > Testing > ------- > > I have written unit tests. I will also test externally by setting up > everything > > > Thanks, > > pavan kumar kolamuri > >
