falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pallavi Rao" <pallavi....@inmobi.com>
Subject Re: Review Request 39588: State Store for instances scheduled by Falcon Native Scheduler
Date Mon, 26 Oct 2015 11:35:02 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message