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 Mon, 23 Nov 2015 06:48:21 GMT


> On Nov. 22, 2015, 5:38 p.m., Ajay Yadava wrote:
> > scheduler/src/test/java/org/apache/falcon/notification/service/AlarmServiceTest.java,
line 61
> > <https://reviews.apache.org/r/40439/diff/1/?file=1129479#file1129479line61>
> >
> >     It was not required as we were dealing with only entityID.
> 
> pavan kumar kolamuri wrote:
>     Then please remove this commented line

Sure, will remove during the commit.


> On Nov. 22, 2015, 5:38 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/EntityID.java, line 28
> > <https://reviews.apache.org/r/40439/diff/1/?file=1129468#file1129468line28>
> >
> >     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.
> 
> pavan kumar kolamuri wrote:
>     Instead of adding EntityClusterID , for the EntityID itself add cluster name also
as key and we are done right ? We can use the same every where and there won't be any confusion.
Please correct me if i miss something here ?

Firstly to emphasise it again, I haven't "added" a new type of ID, it already exists, I have
just put it in a different subclass. Using entityclusterid will always require a cluster and
can be incorrect or inefficient in some cases e.g. to check if an entity exists we shouldn't
need to pass a cluster


- Ajay


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


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