falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "pavan kumar kolamuri" <pavan.kolam...@gmail.com>
Subject Re: Review Request 39588: State Store for instances scheduled by Falcon Native Scheduler
Date Tue, 03 Nov 2015 17:42:05 GMT


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


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