falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ajay Yadava" <ajayn...@gmail.com>
Subject Re: Review Request 35724: Base framework of the native scheduler
Date Mon, 19 Oct 2015 04:51:41 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message