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