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, 14 Sep 2015 19:09:22 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35724/#review98751
-----------------------------------------------------------



common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 257)
<https://reviews.apache.org/r/35724/#comment155597>

    This is incorrect, reason being that for certain frequencies like monthly the time in
millis between each run keeps changing. This will fail e.g. for monthly feeds.



scheduler/pom.xml (line 92)
<https://reviews.apache.org/r/35724/#comment155332>

    Any reasons to not use 2.2.1 which is the latest?



scheduler/pom.xml (line 103)
<https://reviews.apache.org/r/35724/#comment155333>

    1.10.19 is the latest.



scheduler/pom.xml (line 109)
<https://reviews.apache.org/r/35724/#comment155336>

    why not 2.8.2?



scheduler/src/main/java/org/apache/falcon/exception/DAGEngineException.java (line 23)
<https://reviews.apache.org/r/35724/#comment155337>

    Description seems to be incorrect.



scheduler/src/main/java/org/apache/falcon/notification/service/FalconNotificationService.java
(line 48)
<https://reviews.apache.org/r/35724/#comment155606>

    Incorrect javadoc, it is actually for registering.



scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java (line
33)
<https://reviews.apache.org/r/35724/#comment155602>

    numInstances and it's getter setters are not being used. Please remove it. If you want
for future then still delete it and file a JIRA.



scheduler/src/main/java/org/apache/falcon/notification/service/event/JobCompletedEvent.java
(line 28)
<https://reviews.apache.org/r/35724/#comment155593>

    Remove stray todo statement from all classes.



scheduler/src/main/java/org/apache/falcon/notification/service/event/JobCompletedEvent.java
(line 37)
<https://reviews.apache.org/r/35724/#comment155603>

    Constructor allows construction of incomplete object, which is not a good practice.



scheduler/src/main/java/org/apache/falcon/notification/service/event/JobScheduledEvent.java
(line 30)
<https://reviews.apache.org/r/35724/#comment155604>

    All constructors should form complete objects, i.e. all mandatory parameters should be
available after construction. Please make this change for all the Event Classes



scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java
(line 30)
<https://reviews.apache.org/r/35724/#comment155546>

    Remove stray TODOs



scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java
(line 37)
<https://reviews.apache.org/r/35724/#comment155554>

    This field and it's getters and setters are unused. Please remove it.



scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java
(line 43)
<https://reviews.apache.org/r/35724/#comment155548>

    not used. Even if it is for future use cases then please remove it for now and file a
JIRA so that it doesn't get dropped.



scheduler/src/main/java/org/apache/falcon/notification/service/request/NotificationRequest.java
(line 38)
<https://reviews.apache.org/r/35724/#comment155608>

    ORDER is not being set in any of the subclasses.



scheduler/src/main/java/org/apache/falcon/notification/service/request/TimeNotificationRequest.java
(line 31)
<https://reviews.apache.org/r/35724/#comment155609>

    Builder pattern should not be used for supplying mandatory patterns as it allows creation
of incomplete objects. It should be used only for optional parameters. Please change this
from all NotificationRequest subclasses.



scheduler/src/main/java/org/apache/falcon/state/EntityState.java (line 71)
<https://reviews.apache.org/r/35724/#comment155599>

    Incorrect message.



scheduler/src/main/java/org/apache/falcon/state/ID.java (line 33)
<https://reviews.apache.org/r/35724/#comment155590>

    ID is overloaded for both entity key and instance key. Both should have separate classes
as the mandatory parameters for both of them are different.



scheduler/src/main/java/org/apache/falcon/state/ID.java (line 145)
<https://reviews.apache.org/r/35724/#comment155592>

    toString seems to be used for InstanceKey this should be separated out.



scheduler/src/main/java/org/apache/falcon/state/ID.java (line 189)
<https://reviews.apache.org/r/35724/#comment155589>

    This is incorrect. compareTo should be called only on objects of same/comparable type
and not on generic object. If you use the parameterized version of " Comparator<ID>"
this will give you compile time error.


- Ajay Yadava


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


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