----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39588/#review103989 -----------------------------------------------------------
scheduler/src/main/java/org/apache/falcon/state/store/ValidateConnectionBean.java (line 31) <https://reviews.apache.org/r/39588/#comment162165> Why do we need this? Especially since we are anyway validating against EntityBean and InstanceBean, can't we use one of them? scheduler/src/main/java/org/apache/falcon/state/store/service/FalconJPAService.java (line 44) <https://reviews.apache.org/r/39588/#comment162168> Have the prefix as "falcon.statestore.jdbc" and modify the the below constants to remove any additional jdbc prefix. scheduler/src/main/java/org/apache/falcon/state/store/service/FalconJPAService.java (line 88) <https://reviews.apache.org/r/39588/#comment162169> This class will need to be registered as a service in startup.properties for this method to get invoked. scheduler/src/main/java/org/apache/falcon/state/store/service/FalconJPAService.java (line 114) <https://reviews.apache.org/r/39588/#comment162170> May be a concern if you expect admins to specify the DB password in clear text. May be we can add an option to also get it from the command prompt (admin types it in), if the password is not specified. scheduler/src/main/java/org/apache/falcon/state/store/service/FalconJPAService.java (line 171) <https://reviews.apache.org/r/39588/#comment162172> Can't we cache the entity manager? Every call of PersistentStateStore invokes getEntityManager; creating every time will be expensive. scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java (line 53) <https://reviews.apache.org/r/39588/#comment162175> Why Derby DB alone? As long as it has a JDBC driver, it should be fine. right? In our production env., we will use MySQL. scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java (line 64) <https://reviews.apache.org/r/39588/#comment162176> Nit : Generate SQL script instead of ... scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java (line 255) <https://reviews.apache.org/r/39588/#comment162179> The tables may exist, but, may not be of for the current version of Falcon. What do we do in such a case? We should at the least verify the version and message out accordingly. scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java (line 379) <https://reviews.apache.org/r/39588/#comment162178> Need to check for instance table too? scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java (line 399) <https://reviews.apache.org/r/39588/#comment162180> Code branching can be improved to have this line only once. scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java (line 405) <https://reviews.apache.org/r/39588/#comment162177> Falcon DB Tool and Falcon version are same. isn't it? If so, we can just say "Falcon server version:" scheduler/src/main/resources/META-INF/persistence.xml (line 44) <https://reviews.apache.org/r/39588/#comment162167> With this setting and the fact that we are not modifying the result of a query, I think the queries can be optimized for read and don't need a transational boundary. Ties back to my comment on PersistentStateStore. scheduler/src/test/java/org/apache/falcon/state/EntityStateServiceTest.java (line 48) <https://reviews.apache.org/r/39588/#comment162181> This test just tests the valid transitions of state of an entity. It should not require any persistence. Please remove the DB dependency and let it use the InMemory DB. src/conf/startup.properties (line 253) <https://reviews.apache.org/r/39588/#comment162182> Missing JPAService? Also, as mentioned, we can keep this commented and allow users to uncomment it if they use native scheduler. - Pallavi Rao 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 > >
