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 Thu, 12 Nov 2015 13:36:37 GMT


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > common/src/main/resources/startup.properties, line 271
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114729#file1114729line271>
> >
> >     Can you please add one line of documentation along with some of the not so obvious
properties?

Sure will add


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java, line
45
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114732#file1114732line45>
> >
> >     Just using DateTimeZone.UTC is sufficient.

Yes looks like we can use it . I am not sure why it was added like this.


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java, line
50
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114732#file1114732line50>
> >
> >     It will be useful to elaborate what exactly creationTime means and how it is
different from instanceTime.

sure will add comments


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java, line
173
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114732#file1114732line173>
> >
> >     Is this the primary key?

Yes for Execution instance this is unique


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java,
line 63
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114734#file1114734line63>
> >
> >     Is the order important?

Yes , in code we have to compare awaited predicates.


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java,
line 243
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114734#file1114734line243>
> >
> >     If the order is important as above won't passing it other implementation like
HashSet cause issues?

In HashSet order won't be guaranteed. Yes we need sorting thats why using tree set.


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 39
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114735#file1114735line39>
> >
> >     Make it Comparable<Predicate> for extra type check.

Sure makes sense


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 42
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114735#file1114735line42>
> >
> >     I can imagine the need to define equals but ordering predicates usecase is not
clear to me. Can you please explain?

Ajay , there are few places where two processExecutionInstances comparision required to remove
from list etc. As Processexecution intance has awaiting predicates. As we have to compare
awaiting predicates they have to be compared after sorting . Thats why tree set is used for
awaiting predicates.


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 46
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114735#file1114735line46>
> >
> >     Since you are accepting Object as argument, you should do a type check before
trying to cast it to Predicate.

Ya will fix.


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 65
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114735#file1114735line65>
> >
> >     Comparing sets won't be sufficient?

Yes we have to compare two sets of Predicates . Predicate is a object which contains Type
and Map of clauses. I didn't find better solution for this . Can you please suggest alternate
solution ?


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 83
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114735#file1114735line83>
> >
> >     I know this is not being introduced in this JIRA but can we rename this enum
to PredicateType please? Type is too generic.

I think this is ok as it is present in Predicate Class. We will use this as Predicate.TYPE


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/EntityStateStore.java, line
82
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114738#file1114738line82>
> >
> >     This is a potentially risky method. I can't imagine a scenario where it will
be useful in production environments. Is it just for tests?

Yes it is just for tests


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/InstanceStateStore.java, line
124
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114740#file1114740line124>
> >
> >     Same as above.

Yes it is only for tests. Yes it is risky method . But coulnd find alternative for tests


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 53
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line53>
> >
> >     "Entity instance" had me confused for a moment.

sure will change


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 105
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114735#file1114735line105>
> >
> >     No need to override if you are inheriting the same behavior, right?

Yes we will discuss this with you offline


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 107
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114735#file1114735line107>
> >
> >     I think this is not what you want. Two objects which are equal must return the
same hashcode, which is not guaranteed by this implementation.

As i said above will explain you about this


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 293
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114735#file1114735line293>
> >
> >     Interesting. I strongly suspect that all this is required only because of that
one wrong hashcode implementation :)

Agreed will discuss with this offline


> On Nov. 4, 2015, 6:44 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 305
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114735#file1114735line305>
> >
> >     Why is this method called evaluate and not equals? Are both required?

same as above


- pavan kumar


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


On Nov. 3, 2015, 5:45 p.m., pavan kumar kolamuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39588/
> -----------------------------------------------------------
> 
> (Updated Nov. 3, 2015, 5:45 p.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 6f2c480 
>   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/EntityStateStore.java 4aa6fdb

>   scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java 3822860

>   scheduler/src/main/java/org/apache/falcon/state/store/InstanceStateStore.java d6a4b49

>   scheduler/src/main/java/org/apache/falcon/state/store/StateStore.java f595c26 
>   scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java PRE-CREATION

>   scheduler/src/main/java/org/apache/falcon/state/store/jdbc/EntityBean.java PRE-CREATION

>   scheduler/src/main/java/org/apache/falcon/state/store/jdbc/InstanceBean.java PRE-CREATION

>   scheduler/src/main/java/org/apache/falcon/state/store/jdbc/JDBCStateStore.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/TestJDBCStateStore.java
PRE-CREATION 
>   scheduler/src/test/java/org/apache/falcon/tools/TestFalconStateStoreDBCLI.java PRE-CREATION

>   scheduler/src/test/resources/startup.properties PRE-CREATION 
>   src/bin/falcon-db.sh PRE-CREATION 
>   src/conf/startup.properties ce6e91f 
>   src/main/assemblies/distributed-package.xml 794eaef 
>   src/main/assemblies/standalone-package.xml fcff8d7 
>   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