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