> On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > common/src/main/java/org/apache/falcon/entity/EntityUtil.java, line 1041 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097275#file1097275line1041> > > > > This function assumes that all jobs for an entity run with same > > priority, which is not true in case of feed.
Ok. So, a single method cannot handle both process and Feed as stage will also be an input for Feed. I modified this method to return priority only for Process. We can have a separate method to return the priority for a stage of a feed. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/exception/StateStoreException.java, > > line 25 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097281#file1097281line25> > > > > Since there are several state stores, can we disambiguate it by > > something like "SchedulerStateStoreException"? There shouldn't be multiple state stores. There is config store, there is a graph store and there should only be one state store. Hence the name. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/EntityExecutor.java, > > line 50 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097282#file1097282line50> > > > > This class contains the get(..) method and it is also inherited by > > ProcessExecutor and FeedExecutor which also inherits this. > > > > Getting the relevant exectuor should not be part of the executor and > > this inheritance seems counter intuitive. Agreed. Removed that method and moved creation to FalconExecutionService. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java, > > line 36 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097283#file1097283line36> > > > > Can you please create a JIRA for all the pending TODOs and remove them > > from the code? This will done when we do the state store (FALCON 1234) where we'll separate the ExecutionInstance bean and InstanceExecutor. But, would still to keep the todo around as a quick note to whoever implements this. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java, > > line 62 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097284#file1097284line62> > > > > There are two ways of doing the same thing - get an entity executor. > > 1. EntityExecutor.get(..) > > 2. getEntityExecutor(..) > > > > There should actually be one way of doing it which takes care of > > caching and initialization both. Otherwise caching advantage is not > > guaranteed, right? Agreed. I would like to have creation and retrieval separate, hence, have 2 methods. But, yes, agreed and removed EntityExecutor.get() method. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java, > > line 71 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097284#file1097284line71> > > > > create JIRA and remove todo? Part of the migration JIRA (FALCON 1233), but, would still like to keep the TODO around as quick note till it is addressed. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 121 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097286#file1097286line121> > > > > please create JIRA and remove TODO Part of the data notification service JIRA (FALCON 1230), but, would still like to keep the TODO around as quick note till it is addressed. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 139 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097286#file1097286line139> > > > > Please create JIRA and remove TODO. Unnecessary todo, as this was taken care of. Removed TODO > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 144 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097286#file1097286line144> > > > > Please create JIRA and remove TODO. Unnecessary todo, as this was taken care of. Removed TODO > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 276 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097286#file1097286line276> > > > > There should be a way to unregister from all notifications. Correct. There was already a method. Somehow missed using it here. Updated. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/pom.xml, line 57 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097277#file1097277line57> > > > > Why is this dependency required? The Builders are still part of that package and hence the dependency. Will take up the refactor a little later. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java, > > line 73 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097287#file1097287line73> > > > > Please create JIRA and remove TODO. Removed TODO as it is no longer relevant. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java, > > line 73 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097287#file1097287line73> > > > > Please remove TODO and create JIRA if needed. Package is different. So, should be fine. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java, > > line 369 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097287#file1097287line369> > > > > Use ProcessHelper.getCluster() method instead. Totally makes sense. Made the change. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/AlarmService.java, > > line 95 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097296#file1097296line95> > > > > Please remove TODO and create JIRA. No functionality is affected to add a JIRA. Right now there is no support in quartz. We should just use Quartz when the support is added. Just updated the TODO for future. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/AlarmService.java, > > line 104 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097296#file1097296line104> > > > > Delay should be calculated from lastInstanceTime instead of > > currentTime, otherwise it can result in an error. Good Catch. Fixed it. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/StateStore.java, line > > 33 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097315#file1097315line33> > > > > Please remove TODO and create JIRA. JIRA already present (FALCON-1234). Just a handy note for whoever implements it. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/state/ID.java, line 33 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097308#file1097308line33> > > > > This class should be subclassed into two classes. > > 1. EntityId > > 2. InstanceId > > > > This will ensure that the objects are always well constructed. > > > > It will also make it easier for use instead of remembering which method > > to use. > > > > It will improve readability and modularity. Agree that it is bit overloaded. I tried separating this out into EntityID and InstanceID (as subclasses of ID) and the code got a lot messier, with instanceof checks all over the place as the notification services accept ID (and that I don't want to change). I made the changes and had to revert becaue the checks to see which sub-class of the ID was meant to be used in that place. So, prefer to keep it this way for now. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/state/store/StateStore.java, line > > 34 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097315#file1097315line34> > > > > We should divide it into separate interfaces(entity related, instance > > related) and finally extend one interface from all of them to be > > implemented by all implementations. This will make the code more manageable. Makes sense. Done. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngine.java, > > line 32 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097316#file1097316line32> > > > > This looks like hugely overlapping with AbstractWorkflowEngine (if not > > duplicate) Yes. It is because this is the interface that talks to a DAG Engine and the only DAG Engine we know of and need to support right now is Oozie. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngine.java, > > line 105 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097316#file1097316line105> > > > > Is it always specific to Oozie? Nope. Fixed it. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngineFactory.java, > > line 43 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097317#file1097317line43> > > > > So there can never be two DAGEngines for a cluster? Why is this a > > restriction? A DAG Engine can only be tied to a cluster mainly because the engine endpoints (such as oozie) is currently abstracted out at the cluster level. Individual entities do not specify the endpoints. An entity can belong to multiple clusters though if 2 different DAG Engines need to be used. Just preserving the current semantics. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java, > > line 133 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097318#file1097318line133> > > > > Please create JIRA and remove TODO. Warrants a new JIRA. Created FALCON-1547. > On Oct. 19, 2015, 4:51 a.m., Ajay Yadava wrote: > > scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngineFactory.java, > > line 31 > > <https://reviews.apache.org/r/35724/diff/3/?file=1097317#file1097317line31> > > > > This property is not defined in startup.properties. I think this will > > throw an error in runtime. Will fail during startup itself because the Services that use this are initialized during startup. - Pallavi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35724/#review102859 ----------------------------------------------------------- On Oct. 13, 2015, 11:53 a.m., Pallavi Rao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35724/ > ----------------------------------------------------------- > > (Updated Oct. 13, 2015, 11:53 a.m.) > > > Review request for Falcon and Srikanth Sundarrajan. > > > Bugs: FALCON-1213 > https://issues.apache.org/jira/browse/FALCON-1213 > > > Repository: falcon-git > > > Description > ------- > > The patch has the basic framework for the scheduler. Each of the individual > service needs to be implemented completely and will be done as separate > JIRAs. The intention of the patch is ensure the base framework satisfies all > use cases and get any early feedback in terms of course correction. > > > Diffs > ----- > > common/src/main/java/org/apache/falcon/entity/EntityUtil.java 3ab9339 > pom.xml 54e6cd1 > scheduler/pom.xml PRE-CREATION > scheduler/src/main/java/org/apache/falcon/exception/DAGEngineException.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/exception/InvalidStateTransitionException.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/exception/ServiceException.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/exception/StateStoreException.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/execution/EntityExecutor.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/execution/NotificationHandler.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/execution/SchedulerUtil.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/FalconNotificationService.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/NotificationServicesRegistry.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/event/Event.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/event/JobCompletedEvent.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/event/JobScheduledEvent.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/event/TimeElapsedEvent.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/AlarmService.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/JobCompletionService.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/request/AlarmRequest.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/request/JobCompletionNotificationRequest.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/request/JobScheduleNotificationRequest.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/notification/service/request/NotificationRequest.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/EntityState.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/state/EntityStateChangeHandler.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/ID.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/InstanceState.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/state/InstanceStateChangeHandler.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/StateMachine.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/StateService.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/state/store/AbstractStateStore.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/store/StateStore.java > PRE-CREATION > scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngine.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngineFactory.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java > PRE-CREATION > > scheduler/src/main/java/org/apache/falcon/workflow/engine/OozieDAGEngine.java > PRE-CREATION > > scheduler/src/test/java/org/apache/falcon/execution/FalconExecutionServiceTest.java > PRE-CREATION > scheduler/src/test/java/org/apache/falcon/execution/MockDAGEngine.java > PRE-CREATION > scheduler/src/test/java/org/apache/falcon/execution/SchedulerUtilTest.java > PRE-CREATION > > scheduler/src/test/java/org/apache/falcon/notification/service/AlarmServiceTest.java > PRE-CREATION > > scheduler/src/test/java/org/apache/falcon/notification/service/SchedulerServiceTest.java > PRE-CREATION > scheduler/src/test/java/org/apache/falcon/predicate/PredicateTest.java > PRE-CREATION > scheduler/src/test/java/org/apache/falcon/state/EntityStateServiceTest.java > PRE-CREATION > > scheduler/src/test/java/org/apache/falcon/state/InstanceStateServiceTest.java > PRE-CREATION > scheduler/src/test/resources/config/cluster/cluster-0.1.xml PRE-CREATION > scheduler/src/test/resources/config/feed/feed-0.1.xml PRE-CREATION > scheduler/src/test/resources/config/process/process-0.1.xml PRE-CREATION > webapp/pom.xml e63aa44 > > Diff: https://reviews.apache.org/r/35724/diff/ > > > Testing > ------- > > New UTs have been added. > > > Thanks, > > Pallavi Rao > >
