You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by pavan kumar kolamuri <pa...@gmail.com> on 2015/08/28 14:14:30 UTC
Re: Review Request 35724: Base framework of the native scheduler
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35724/#review96853
-----------------------------------------------------------
scheduler/pom.xml (line 73)
<https://reviews.apache.org/r/35724/#comment152533>
Snapshots should not be used right ?
scheduler/src/main/java/org/apache/falcon/execution/EntityExecutor.java (line 33)
<https://reviews.apache.org/r/35724/#comment152534>
Should this extend InstanceChangeHandler also instead of process executor ? since FeedExecutor might also need this
scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java (line 34)
<https://reviews.apache.org/r/35724/#comment152537>
IMO, as exection instance is an object to be stored in Data store. If we add InstanceExecutor handles all notifications it will be good.
scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 58)
<https://reviews.apache.org/r/35724/#comment152539>
I have checked the flow , flow from Instance Event Trigger to Instance Event Conditions Met is missing when there are awaited Conditions. Please correct me if i am missing something
scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 449)
<https://reviews.apache.org/r/35724/#comment152540>
Shouldn't be order reverse ? First invalidate and then destroy right
scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java (line 184)
<https://reviews.apache.org/r/35724/#comment152543>
Few variables are unused like numRetries,retryDelayInMillis. Are these variables used for future todo's ?
scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java (line 220)
<https://reviews.apache.org/r/35724/#comment152542>
I don't think this method is called. Means the case where awaitedInstances queue is full is missing. Can you please check
scheduler/src/main/java/org/apache/falcon/notification/service/impl/TimeNotificationService.java (line 65)
<https://reviews.apache.org/r/35724/#comment152545>
Shouldn't there be deletion also for notifications once that notification is completed.
scheduler/src/test/java/org/apache/falcon/execution/FalconExecutionServiceTest.java (line 165)
<https://reviews.apache.org/r/35724/#comment152538>
In this test case there are places we should get the instance value from StateStore to get the updated status
- pavan kumar kolamuri
On July 28, 2015, 11:07 a.m., Pallavi Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35724/
> -----------------------------------------------------------
>
> (Updated July 28, 2015, 11:07 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/pom.xml 36de1f5
> common/src/main/java/org/apache/falcon/entity/EntityUtil.java b86d9d7
> common/src/main/java/org/apache/falcon/workflow/WorkflowJobEndNotificationService.java c4f8843
> common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java cfdc84d
> pom.xml 31997e8
> 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/notification/service/FalconNotificationService.java PRE-CREATION
> scheduler/src/main/java/org/apache/falcon/notification/service/ServicesRegistry.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/DataNotificationService.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/impl/TimeNotificationService.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/notification/service/request/TimeNotificationRequest.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/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/notification/service/SchedulerServiceTest.java PRE-CREATION
> scheduler/src/test/java/org/apache/falcon/notification/service/TimeNotificationServiceTest.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
>
> Diff: https://reviews.apache.org/r/35724/diff/
>
>
> Testing
> -------
>
> New UTs have been added.
>
>
> Thanks,
>
> Pallavi Rao
>
>
Re: Review Request 35724: Base framework of the native scheduler
Posted by Pallavi Rao <pa...@inmobi.com>.
> On Aug. 28, 2015, 12:14 p.m., pavan kumar kolamuri wrote:
> > scheduler/pom.xml, line 73
> > <https://reviews.apache.org/r/35724/diff/2/?file=1023467#file1023467line73>
> >
> > Snapshots should not be used right ?
Yes. My bad. Fixed it.
> On Aug. 28, 2015, 12:14 p.m., pavan kumar kolamuri wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/EntityExecutor.java, line 33
> > <https://reviews.apache.org/r/35724/diff/2/?file=1023472#file1023472line33>
> >
> > Should this extend InstanceChangeHandler also instead of process executor ? since FeedExecutor might also need this
Good catch.
> On Aug. 28, 2015, 12:14 p.m., pavan kumar kolamuri wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java, line 449
> > <https://reviews.apache.org/r/35724/diff/2/?file=1023477#file1023477line449>
> >
> > Shouldn't be order reverse ? First invalidate and then destroy right
Once we invalidate (the cache), the executor will not hold the reference to that instance. Hence, it is destroy and then invalidate.
> On Aug. 28, 2015, 12:14 p.m., pavan kumar kolamuri wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java, line 184
> > <https://reviews.apache.org/r/35724/diff/2/?file=1023487#file1023487line184>
> >
> > Few variables are unused like numRetries,retryDelayInMillis. Are these variables used for future todo's ?
Those were intended to be used for failure scenarios when for some reason we couldn't schedule it on the DAG Engine. Since the implementation for that incomplete, I'll remove them. Will introduce them later when we implement failure handling.
> On Aug. 28, 2015, 12:14 p.m., pavan kumar kolamuri wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java, line 220
> > <https://reviews.apache.org/r/35724/diff/2/?file=1023487#file1023487line220>
> >
> > I don't think this method is called. Means the case where awaitedInstances queue is full is missing. Can you please check
The awaitedInstances cache has a size limit. This method will be called by the cacheBuilder library, when any instance is removed from the cache because it exceeded the max. size.
> On Aug. 28, 2015, 12:14 p.m., pavan kumar kolamuri wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/TimeNotificationService.java, line 65
> > <https://reviews.apache.org/r/35724/diff/2/?file=1023488#file1023488line65>
> >
> > Shouldn't there be deletion also for notifications once that notification is completed.
You are right. Added it in the unregisterForNotification method.
> On Aug. 28, 2015, 12:14 p.m., pavan kumar kolamuri wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java, line 58
> > <https://reviews.apache.org/r/35724/diff/2/?file=1023477#file1023477line58>
> >
> > I have checked the flow , flow from Instance Event Trigger to Instance Event Conditions Met is missing when there are awaited Conditions. Please correct me if i am missing something
Any awaited predicates will be handled by the notify(Event event) of ProcessExecutionInstance
> On Aug. 28, 2015, 12:14 p.m., pavan kumar kolamuri wrote:
> > scheduler/src/test/java/org/apache/falcon/execution/FalconExecutionServiceTest.java, line 165
> > <https://reviews.apache.org/r/35724/diff/2/?file=1023508#file1023508line165>
> >
> > In this test case there are places we should get the instance value from StateStore to get the updated status
Believe you have already made these changes as part of state store impl. Thanks.
> On Aug. 28, 2015, 12:14 p.m., pavan kumar kolamuri wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java, line 34
> > <https://reviews.apache.org/r/35724/diff/2/?file=1023473#file1023473line34>
> >
> > IMO, as exection instance is an object to be stored in Data store. If we add InstanceExecutor handles all notifications it will be good.
Agreed. But, I want to handle that in a separate JIRA that refactors this code so it caters well to the state store too.
- Pallavi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35724/#review96853
-----------------------------------------------------------
On July 28, 2015, 11:07 a.m., Pallavi Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35724/
> -----------------------------------------------------------
>
> (Updated July 28, 2015, 11:07 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/pom.xml 36de1f5
> common/src/main/java/org/apache/falcon/entity/EntityUtil.java b86d9d7
> common/src/main/java/org/apache/falcon/workflow/WorkflowJobEndNotificationService.java c4f8843
> common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java cfdc84d
> pom.xml 31997e8
> 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/notification/service/FalconNotificationService.java PRE-CREATION
> scheduler/src/main/java/org/apache/falcon/notification/service/ServicesRegistry.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/DataNotificationService.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/impl/TimeNotificationService.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/notification/service/request/TimeNotificationRequest.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/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/notification/service/SchedulerServiceTest.java PRE-CREATION
> scheduler/src/test/java/org/apache/falcon/notification/service/TimeNotificationServiceTest.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
>
> Diff: https://reviews.apache.org/r/35724/diff/
>
>
> Testing
> -------
>
> New UTs have been added.
>
>
> Thanks,
>
> Pallavi Rao
>
>