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 40439: Native Scheduler - Code Refactoring: Refactor ID into more specific sub classes
Date Sun, 22 Nov 2015 17:38:12 GMT

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



scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java (line 119)
<https://reviews.apache.org/r/40439/#comment166656>

    Yes. if event.getTarget() is of type EntityID



scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 293)
<https://reviews.apache.org/r/40439/#comment166657>

    It's not guaranteed. Moreover removing it will make it fragile if the behavior changed
in future.



scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java
(line 88)
<https://reviews.apache.org/r/40439/#comment166660>

    :)
    
    It is always EntityClusterID. Even when we get instanceID (in case of instance dependencies)
we find it's EntityClusterID and use that. The interesting side effect is that in it's current
form the instance dependency feature works only for instances of same entity.
    
    This is exactly the kind of clarity which comes when we know what kind of id are we dealing
with and the motivation for this change.



scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java
(line 197)
<https://reviews.apache.org/r/40439/#comment166662>

    As I explained above, it's always EntityClusterID and hence it is correct.



scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java

<https://reviews.apache.org/r/40439/#comment166663>

    I know but it is not removed, just moved. There is also a unit test for concurrency which
can corraborate that the functionality is maintained.



scheduler/src/main/java/org/apache/falcon/state/EntityClusterID.java (line 28)
<https://reviews.apache.org/r/40439/#comment166664>

    It's not my decision but something that already exists in the code. I have just refactored
it which makes it clearer. I understand the confusion but the same confusion exists even without
this change, just that it is not so glaring.



scheduler/src/main/java/org/apache/falcon/state/EntityID.java (line 28)
<https://reviews.apache.org/r/40439/#comment166665>

    From the top of my head I can tell that operations like entity status changes etc. are
being done using EntityID while execution related changes deal with EntityClusterID. A "usage
search" in IDE may reveal more than I can recall right now.



scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java (line 145)
<https://reviews.apache.org/r/40439/#comment166666>

    EntityStates use EntityClusterID only as is evident from earlier change also in creating
the entity (both entity and cluster are passed in the constructor)



scheduler/src/test/java/org/apache/falcon/notification/service/AlarmServiceTest.java (line
61)
<https://reviews.apache.org/r/40439/#comment166668>

    It was not required as we were dealing with only entityID.


- Ajay Yadava


On Nov. 18, 2015, 12:33 p.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40439/
> -----------------------------------------------------------
> 
> (Updated Nov. 18, 2015, 12:33 p.m.)
> 
> 
> Review request for Falcon.
> 
> 
> Bugs: FALCON-1607
>     https://issues.apache.org/jira/browse/FALCON-1607
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Currently the file ID.java is used to uniquely identify various "entities" for native
scheduler. This class is overloaded and serves multiple tasks like getting an entity id for
an entity and an instance id for an instance. Keeping all this code in one class creates various
issues like no check on object creation - one can accidentally call an instance id when the
underlying object was supposed to be representing an entity etc. Since ID represents the unique
identifier for an instance, entity etc. most methods pass ID and this makes the code hard
to reason as we don't know what are we dealing with - an entity or an instance or something
else.
> 
> 
> Diffs
> -----
> 
>   scheduler/src/main/java/org/apache/falcon/execution/EntityExecutor.java 9b07b9e 
>   scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java 3869ff2

>   scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java b959320

>   scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java 8c84f2b

>   scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java d10d2fd 
>   scheduler/src/main/java/org/apache/falcon/notification/service/impl/JobCompletionService.java
73a4199 
>   scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java
a70bc3c 
>   scheduler/src/main/java/org/apache/falcon/state/EntityClusterID.java PRE-CREATION 
>   scheduler/src/main/java/org/apache/falcon/state/EntityID.java PRE-CREATION 
>   scheduler/src/main/java/org/apache/falcon/state/ID.java 420c856 
>   scheduler/src/main/java/org/apache/falcon/state/InstanceID.java PRE-CREATION 
>   scheduler/src/main/java/org/apache/falcon/state/InstanceState.java 8cf24ee 
>   scheduler/src/main/java/org/apache/falcon/state/StateService.java 81357a4 
>   scheduler/src/main/java/org/apache/falcon/state/store/AbstractStateStore.java ba3d5fd

>   scheduler/src/main/java/org/apache/falcon/state/store/EntityStateStore.java 4aa6fdb

>   scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java 3822860

>   scheduler/src/main/java/org/apache/falcon/state/store/InstanceStateStore.java d6a4b49

>   scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java
8dcf3a5 
>   scheduler/src/test/java/org/apache/falcon/execution/FalconExecutionServiceTest.java
b2f9e59 
>   scheduler/src/test/java/org/apache/falcon/notification/service/AlarmServiceTest.java
36f1fd1 
>   scheduler/src/test/java/org/apache/falcon/notification/service/SchedulerServiceTest.java
b4a0f35 
>   scheduler/src/test/java/org/apache/falcon/predicate/PredicateTest.java 95dd5ae 
>   scheduler/src/test/java/org/apache/falcon/state/InstanceStateServiceTest.java d27ac7e

> 
> Diff: https://reviews.apache.org/r/40439/diff/
> 
> 
> Testing
> -------
> 
> Just refactored, all existing unit tests pass.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>


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