falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ajay Yadava" <ajayn...@gmail.com>
Subject Re: Review Request 39588: State Store for instances scheduled by Falcon Native Scheduler
Date Wed, 04 Nov 2015 18:44:49 GMT

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



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

    Can you please add one line of documentation along with some of the not so obvious properties?



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

    Just using DateTimeZone.UTC is sufficient.



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

    It will be useful to elaborate what exactly creationTime means and how it is different
from instanceTime.



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

    Is this the primary key?



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

    Is the order important?



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

    If the order is important as above won't passing it other implementation like HashSet
cause issues?



scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 39)
<https://reviews.apache.org/r/39588/#comment163443>

    Make it Comparable<Predicate> for extra type check.



scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 42)
<https://reviews.apache.org/r/39588/#comment163496>

    I can imagine the need to define equals but ordering predicates usecase is not clear to
me. Can you please explain?



scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 46)
<https://reviews.apache.org/r/39588/#comment163444>

    Since you are accepting Object as argument, you should do a type check before trying to
cast it to Predicate.



scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 65)
<https://reviews.apache.org/r/39588/#comment163498>

    Comparing sets won't be sufficient?



scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 83)
<https://reviews.apache.org/r/39588/#comment163445>

    I know this is not being introduced in this JIRA but can we rename this enum to PredicateType
please? Type is too generic.



scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 84)
<https://reviews.apache.org/r/39588/#comment163447>

    Does a Predicate with type == null make sense or is useful in any scenarios? If not then
we should introduce a constructor to ensure that it is not possible to create a Predicate
without TYPE.



scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 105)
<https://reviews.apache.org/r/39588/#comment163449>

    No need to override if you are inheriting the same behavior, right?



scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 107)
<https://reviews.apache.org/r/39588/#comment163448>

    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.



scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 293)
<https://reviews.apache.org/r/39588/#comment163500>

    Interesting. I strongly suspect that all this is required only because of that one wrong
hashcode implementation :)



scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 305)
<https://reviews.apache.org/r/39588/#comment163502>

    Why is this method called evaluate and not equals? Are both required?



scheduler/src/main/java/org/apache/falcon/state/store/EntityStateStore.java (line 82)
<https://reviews.apache.org/r/39588/#comment163505>

    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?



scheduler/src/main/java/org/apache/falcon/state/store/InstanceStateStore.java (line 124)
<https://reviews.apache.org/r/39588/#comment163510>

    Same as above.



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

    "Entity instance" had me confused for a moment.


- Ajay Yadava


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