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