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 09:25:18 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39588/#review103982
-----------------------------------------------------------



checkstyle/src/main/resources/falcon/checkstyle.xml (line 233)
<https://reviews.apache.org/r/39588/#comment162118>

    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



common/src/main/resources/startup.properties (line 252)
<https://reviews.apache.org/r/39588/#comment162119>

    Should we keep this set of properties commented out and explicitly mentioning that these
are needed only for native scheduler?



pom.xml (line 116)
<https://reviews.apache.org/r/39588/#comment162117>

    Move to 2.4, unless there is a particular dependency on this version.



scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java (line 51)
<https://reviews.apache.org/r/39588/#comment162120>

    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.



scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 88)
<https://reviews.apache.org/r/39588/#comment162147>

    Change all nominalTime to instanceTime. Srikanth was saying that we should avoid the Oozie
nomenclature.



scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 192)
<https://reviews.apache.org/r/39588/#comment162121>

    Nit: When is this null? The instance variable is initialized with an empty array list.
No harm with an additional check though.



scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 312)
<https://reviews.apache.org/r/39588/#comment162122>

    Can you move this to Predicate class as a utility method?



scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 320)
<https://reviews.apache.org/r/39588/#comment162123>

    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.



scheduler/src/main/java/org/apache/falcon/state/StateService.java (line 141)
<https://reviews.apache.org/r/39588/#comment162124>

    Nit: Use isEmpty() instead



scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java (line 46)
<https://reviews.apache.org/r/39588/#comment162134>

    Please add Javadoc comments to the methods.



scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java (line 51)
<https://reviews.apache.org/r/39588/#comment162135>

    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.



scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java (line 67)
<https://reviews.apache.org/r/39588/#comment162133>

    Lets store the definition as clob in the state store, that will avoid consistency issues.



scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java (line 105)
<https://reviews.apache.org/r/39588/#comment162148>

    See if you can avoid default constructor or at least have a constructor with mandatory
parameters and null checks there in.



scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java (line 121)
<https://reviews.apache.org/r/39588/#comment162149>

    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.



scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java (line 206)
<https://reviews.apache.org/r/39588/#comment162151>

    Make the access level private



scheduler/src/main/java/org/apache/falcon/state/store/BeanMapperUtil.java (line 216)
<https://reviews.apache.org/r/39588/#comment162150>

    Throw a OperationNotSupported Exception.



scheduler/src/main/java/org/apache/falcon/state/store/EntityBean.java (line 18)
<https://reviews.apache.org/r/39588/#comment162128>

    Can we move all the JDBC based persistent store classes to org.apache.falcon.state.store.jdbc
?



scheduler/src/main/java/org/apache/falcon/state/store/EntityBean.java (line 33)
<https://reviews.apache.org/r/39588/#comment162153>

    Add constraints, such as not null? For this table, in particular,  none of the columns
can be null.



scheduler/src/main/java/org/apache/falcon/state/store/EntityBean.java (line 43)
<https://reviews.apache.org/r/39588/#comment162127>

    ENTITY_JOBS? May be just call it ENTITIES?



scheduler/src/main/java/org/apache/falcon/state/store/EntityBean.java (line 44)
<https://reviews.apache.org/r/39588/#comment162131>

    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.



scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java (line 115)
<https://reviews.apache.org/r/39588/#comment162142>

    Just calling the clear() method should do it. right?



scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java (line 245)
<https://reviews.apache.org/r/39588/#comment162155>

    @Override ?



scheduler/src/main/java/org/apache/falcon/state/store/InstanceBean.java (line 46)
<https://reviews.apache.org/r/39588/#comment162164>

    Can we use limit, since we are not interested in all instances?



scheduler/src/main/java/org/apache/falcon/state/store/InstanceBean.java (line 50)
<https://reviews.apache.org/r/39588/#comment162145>

    Use INSTANCES, rather than INSTANCE_JOBS.



scheduler/src/main/java/org/apache/falcon/state/store/InstanceBean.java (line 51)
<https://reviews.apache.org/r/39588/#comment162152>

    Should we also add some constraints such not null?



scheduler/src/main/java/org/apache/falcon/state/store/InstanceBean.java (line 54)
<https://reviews.apache.org/r/39588/#comment162157>

    How are we modelling the relationship between instance and entity. Ideally, entity Id
should be a foriegn key.



scheduler/src/main/java/org/apache/falcon/state/store/InstanceBean.java (line 68)
<https://reviews.apache.org/r/39588/#comment162146>

    Remove any variable names with nominalTime. Srikanth was saying that is a Oozie nomenclature
and that we should use instanceTime instead.



scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java (line 40)
<https://reviews.apache.org/r/39588/#comment162154>

    Call it JDBCStateStore to be clear.



scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java (line 53)
<https://reviews.apache.org/r/39588/#comment162156>

    Shouldn't the instances be deleted first?



scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java (line 60)
<https://reviews.apache.org/r/39588/#comment162158>

    Do we really need this check? Won't entityManager.perist() return an error if the row
already exists?



scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java (line 81)
<https://reviews.apache.org/r/39588/#comment162159>

    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?



scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java (line 86)
<https://reviews.apache.org/r/39588/#comment162160>

    Use commitAndClose once and branch on result later.



scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java (line 122)
<https://reviews.apache.org/r/39588/#comment162161>

    Required? Wouldn't update call throw an error, if row exists?



scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java (line 140)
<https://reviews.apache.org/r/39588/#comment162162>

    Required?



scheduler/src/main/java/org/apache/falcon/state/store/PersistentStateStore.java (line 163)
<https://reviews.apache.org/r/39588/#comment162163>

    Required? We should ensure the entity exists. This can be handled by building a 1:M relationship
between Entity and Instance (using foreign key).


- 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