You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Ajay Yadava <aj...@gmail.com> on 2015/11/18 13:33:33 UTC
Review Request 40439: Native Scheduler - Code Refactoring: Refactor
ID into more specific sub classes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40439/
-----------------------------------------------------------
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
Re: Review Request 40439: Native Scheduler - Code Refactoring:
Refactor ID into more specific sub classes
Posted by pavan kumar kolamuri <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40439/#review107583
-----------------------------------------------------------
Ship it!
Ship It!
- pavan kumar kolamuri
On Nov. 23, 2015, 10:32 a.m., Ajay Yadava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40439/
> -----------------------------------------------------------
>
> (Updated Nov. 23, 2015, 10:32 a.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
>
>
Re: Review Request 40439: Native Scheduler - Code Refactoring:
Refactor ID into more specific sub classes
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40439/
-----------------------------------------------------------
(Updated Nov. 23, 2015, 10:32 a.m.)
Review request for Falcon.
Changes
-------
Reverted second patch.
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 (updated)
-----
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
Re: Review Request 40439: Native Scheduler - Code Refactoring:
Refactor ID into more specific sub classes
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40439/
-----------------------------------------------------------
(Updated Nov. 23, 2015, 10:14 a.m.)
Review request for Falcon.
Changes
-------
Addressed the review comments.
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 (updated)
-----
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
Re: Review Request 40439: Native Scheduler - Code Refactoring:
Refactor ID into more specific sub classes
Posted by pavan kumar kolamuri <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40439/#review107307
-----------------------------------------------------------
scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java (line 119)
<https://reviews.apache.org/r/40439/#comment166362>
Is there any case id can be null ?
scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java (line 293)
<https://reviews.apache.org/r/40439/#comment166374>
Its good to have check but in this method event.getTarget is always instanceOf InstanceID right ? Do we need this check ?
scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java (line 88)
<https://reviews.apache.org/r/40439/#comment166381>
Key is not always EntityClusterID here, it can be instanceID also. instanceID is in case of pipeline dependencies and entityId in case of concurrency.
Pallavi please correct us.
scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java (line 197)
<https://reviews.apache.org/r/40439/#comment166388>
I think it is executorAwaitedInstances.get(id) only.
scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java
<https://reviews.apache.org/r/40439/#comment166377>
Why it is removed ? It is required to maintain concurrency of an entity
scheduler/src/main/java/org/apache/falcon/state/EntityClusterID.java (line 28)
<https://reviews.apache.org/r/40439/#comment166398>
Can we maintain either EntityClusterID or EntityID ? is there any issue if we maintain only EntityID ? Its confusing in some places we have EntityID and some places EntityClusterID. In statestore also we are storing entityId in DB and some operations are done using EntityClusterID like startWith in inmemory store
scheduler/src/main/java/org/apache/falcon/state/EntityID.java (line 28)
<https://reviews.apache.org/r/40439/#comment166399>
If both EntityId and EntityClusterID required ? Can you please explain in which cases those are required respectively
scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java (line 145)
<https://reviews.apache.org/r/40439/#comment166395>
Can't we simply use EntityID here ?
scheduler/src/test/java/org/apache/falcon/notification/service/AlarmServiceTest.java (line 61)
<https://reviews.apache.org/r/40439/#comment166394>
why it was commneted ?
- pavan kumar kolamuri
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
>
>
Re: Review Request 40439: Native Scheduler - Code Refactoring:
Refactor ID into more specific sub classes
Posted by pavan kumar kolamuri <pa...@gmail.com>.
> 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.
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 ?
> 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.
Then please remove this commented line
- pavan kumar
-----------------------------------------------------------
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
>
>
Re: Review Request 40439: Native Scheduler - Code Refactoring:
Refactor ID into more specific sub classes
Posted by Ajay Yadava <aj...@gmail.com>.
> 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
>
>
Re: Review Request 40439: Native Scheduler - Code Refactoring:
Refactor ID into more specific sub classes
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
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
>
>