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