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