----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35724/#review102859 -----------------------------------------------------------
There are lots of feedback from earlier review, which are not yet addressed. Can you please address them? common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 1041) <https://reviews.apache.org/r/35724/#comment160653> This function assumes that all jobs for an entity run with same priority, which is not true in case of feed. common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 1055) <https://reviews.apache.org/r/35724/#comment160654> We already support different priorities for different stages in the feed. This may cause correctional issues. common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 1067) <https://reviews.apache.org/r/35724/#comment160655> This should throw an exception because priority doesn't make sense for other entities like cluster. scheduler/pom.xml (line 57) <https://reviews.apache.org/r/35724/#comment160943> Why is this dependency required? scheduler/src/main/java/org/apache/falcon/exception/ServiceException.java (line 25) <https://reviews.apache.org/r/35724/#comment160944> nit: ServiceException sounds too generic and doesn't convey a lot of information about the exception. Can we make it something more specific to scheduler? NotificationServiceException? scheduler/src/main/java/org/apache/falcon/exception/StateStoreException.java (line 25) <https://reviews.apache.org/r/35724/#comment160945> Since there are several state stores, can we disambiguate it by something like "SchedulerStateStoreException"? scheduler/src/main/java/org/apache/falcon/execution/EntityExecutor.java (line 50) <https://reviews.apache.org/r/35724/#comment160960> 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. scheduler/src/main/java/org/apache/falcon/execution/EntityExecutor.java (line 53) <https://reviews.apache.org/r/35724/#comment160946> Will UnsupportedOperationException be more appropriate for this case? scheduler/src/main/java/org/apache/falcon/execution/EntityExecutor.java (line 99) <https://reviews.apache.org/r/35724/#comment160947> Entity and instance level operations are all in one class. Can we please separate entity level operations and instance level operations in different classes? scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java (line 36) <https://reviews.apache.org/r/35724/#comment160948> Can you please create a JIRA for all the pending TODOs and remove them from the code? scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java (line 45) <https://reviews.apache.org/r/35724/#comment160972> Make it final. scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java (line 62) <https://reviews.apache.org/r/35724/#comment160950> 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? scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java (line 71) <https://reviews.apache.org/r/35724/#comment160949> create JIRA and remove todo? scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 121) <https://reviews.apache.org/r/35724/#comment160951> please create JIRA and remove TODO scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 139) <https://reviews.apache.org/r/35724/#comment160952> Please create JIRA and remove TODO. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 144) <https://reviews.apache.org/r/35724/#comment160953> Please create JIRA and remove TODO. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 269) <https://reviews.apache.org/r/35724/#comment160954> Please create JIRA and remove TODO. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 276) <https://reviews.apache.org/r/35724/#comment160955> There should be a way to unregister from all notifications. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 72) <https://reviews.apache.org/r/35724/#comment160977> This method should accept only Process instead of Entity. This will ensure compile time checks. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 73) <https://reviews.apache.org/r/35724/#comment160956> Please create JIRA and remove TODO. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 73) <https://reviews.apache.org/r/35724/#comment160976> Please remove TODO and create JIRA if needed. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 174) <https://reviews.apache.org/r/35724/#comment160957> Please create JIRA and remove TODO. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 174) <https://reviews.apache.org/r/35724/#comment160978> Please remove TODO and create JIRA. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 224) <https://reviews.apache.org/r/35724/#comment160981> Can we separate out the entity and instance level operations? scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 236) <https://reviews.apache.org/r/35724/#comment160982> We should avoid typecasting and should leverage compile time checks as much as possible. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 312) <https://reviews.apache.org/r/35724/#comment160958> Please create JIRA and remove TODO. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 369) <https://reviews.apache.org/r/35724/#comment160979> Use ProcessHelper.getCluster() method instead. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 379) <https://reviews.apache.org/r/35724/#comment160980> Please create JIRA and remove todos. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 380) <https://reviews.apache.org/r/35724/#comment160959> Please create JIRA and remove TODO. scheduler/src/main/java/org/apache/falcon/notification/service/impl/AlarmService.java (line 95) <https://reviews.apache.org/r/35724/#comment160962> Please remove TODO and create JIRA. scheduler/src/main/java/org/apache/falcon/notification/service/impl/AlarmService.java (line 104) <https://reviews.apache.org/r/35724/#comment160963> Delay should be calculated from lastInstanceTime instead of currentTime, otherwise it can result in an error. scheduler/src/main/java/org/apache/falcon/notification/service/impl/JobCompletionService.java (line 58) <https://reviews.apache.org/r/35724/#comment160667> Make it final? scheduler/src/main/java/org/apache/falcon/state/ID.java (line 33) <https://reviews.apache.org/r/35724/#comment160971> 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. scheduler/src/main/java/org/apache/falcon/state/store/StateStore.java (line 33) <https://reviews.apache.org/r/35724/#comment160964> Please remove TODO and create JIRA. scheduler/src/main/java/org/apache/falcon/state/store/StateStore.java (line 34) <https://reviews.apache.org/r/35724/#comment160965> 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. scheduler/src/main/java/org/apache/falcon/state/store/StateStore.java (line 46) <https://reviews.apache.org/r/35724/#comment160966> Passing string is error prone, can we change it to accept an object which represents EntityKey/InstanceKey? This will result in stronger contract. scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngine.java (line 32) <https://reviews.apache.org/r/35724/#comment160968> This looks like hugely overlapping with AbstractWorkflowEngine (if not duplicate) scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngine.java (line 105) <https://reviews.apache.org/r/35724/#comment160967> Is it always specific to Oozie? scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngineFactory.java (line 31) <https://reviews.apache.org/r/35724/#comment160969> This property is not defined in startup.properties. I think this will throw an error in runtime. scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngineFactory.java (line 34) <https://reviews.apache.org/r/35724/#comment160975> Make it final. scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngineFactory.java (line 39) <https://reviews.apache.org/r/35724/#comment160970> Repeated code. The two methods can be refactored to avoid repetition by calling the overloaded version. scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngineFactory.java (line 43) <https://reviews.apache.org/r/35724/#comment160973> So there can never be two DAGEngines for a cluster? Why is this a restriction? scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java (line 133) <https://reviews.apache.org/r/35724/#comment160974> Please create JIRA and remove TODO. - Ajay Yadava 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 > >
