----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39632/#review106556 -----------------------------------------------------------
scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 65) <https://reviews.apache.org/r/39632/#comment165260> Can you please document in detail the purpose of this field? scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 71) <https://reviews.apache.org/r/39632/#comment165259> 2 similar logs in 2 lines, will merging them to one suffice? scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 172) <https://reviews.apache.org/r/39632/#comment165262> scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 140) <https://reviews.apache.org/r/39632/#comment173779> Shouldn't this TODO be removed now? Is there still something left? scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 141) <https://reviews.apache.org/r/39632/#comment173780> What change in polling frequency is required and why? Any reason to not include that in this JIRA itself? scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java (line 33) <https://reviews.apache.org/r/39632/#comment173781> nit: change to plural "dataLocations" scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java (line 43) <https://reviews.apache.org/r/39632/#comment173782> When is a data event invalid? It will be useful to put a comment here as this status is slightly unintuitive for the uninitiated. scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java (line 62) <https://reviews.apache.org/r/39632/#comment173783> nit: convert to plural getDataLocations scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java (line 66) <https://reviews.apache.org/r/39632/#comment173785> nit: Change to plural "setDataLocations" and "locations" scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java (line 74) <https://reviews.apache.org/r/39632/#comment173784> Isn't this always fixed to DATA? scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 63) <https://reviews.apache.org/r/39632/#comment173787> Can you please put a comment explaining which instances are getting ignored and under what circumstances? I am guessing instances get added to ignore list when they unregister. Also instead of ID can you please specify one of the concrete versions? scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 64) <https://reviews.apache.org/r/39632/#comment174078> So how is the state of instancesToIgnore being maintained across restarts? Say someone unregisters for an event, some of the locations are in delayQueue, we add it to instancesToIgnoreList and in the meantime Falcon restarts? What will happen? scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 69) <https://reviews.apache.org/r/39632/#comment173786> excessive logging? scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 155) <https://reviews.apache.org/r/39632/#comment174068> Shouldn't we just reject the requests with null locations instead of accepting them and checking them? I believe then we can get rid of invalid status as well which is unintuitive scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 191) <https://reviews.apache.org/r/39632/#comment174081> Please raise a JIRA for this and delete the TODO. Otherwise no one else knows about the missing capabilities. scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 196) <https://reviews.apache.org/r/39632/#comment174082> What will be callbackID here? Can two different requests have same CallbackID? scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 218) <https://reviews.apache.org/r/39632/#comment174071> nit: getUnavailablePaths (I hate to point grammar mistakes but some time back I realized that the code looks much better after fixing these) scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 220) <https://reviews.apache.org/r/39632/#comment174070> nit: areAllPathsPresent or allPathsExist will be more appropriate IMO scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 221) <https://reviews.apache.org/r/39632/#comment174075> won't "return areAllPathsExist(locations)" suffice? scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 224) <https://reviews.apache.org/r/39632/#comment174072> nit: Don't concatenate the exception object. LOG.error("message", e) should be just fine. scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 226) <https://reviews.apache.org/r/39632/#comment174073> nit: don't concatenate the exception scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 243) <https://reviews.apache.org/r/39632/#comment174074> Casting to boolean shouldn't be required here. May be you need to parameterize pathInfo variable. scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 40) <https://reviews.apache.org/r/39632/#comment174058> What is the boolean for? docs please scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 44) <https://reviews.apache.org/r/39632/#comment174059> nit: isn't this always in Millis and should be accessTimeInMillis as per naming convention of other variables. scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 46) <https://reviews.apache.org/r/39632/#comment174056> I think the datanotification service should be independent of the locationType and this shouldn't be required. scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 47) <https://reviews.apache.org/r/39632/#comment174054> what does isFirst represent? docs please scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 70) <https://reviews.apache.org/r/39632/#comment174053> nit: you can do createdTimeInMillis = accessTime. scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 90) <https://reviews.apache.org/r/39632/#comment174057> Isn't this an internal detail and hence should be private? scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 129) <https://reviews.apache.org/r/39632/#comment174060> Why is a setter method returning this? scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 149) <https://reviews.apache.org/r/39632/#comment174061> nit: same doc for getLocations & getLocationMap?? scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 162) <https://reviews.apache.org/r/39632/#comment174055> Won't returning the difference suffice here? scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 167) <https://reviews.apache.org/r/39632/#comment174063> Should the DataPredicate be concerned about locationType? scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 169) <https://reviews.apache.org/r/39632/#comment174064> I am curious, under what circumstances one will create a data predicate with null paths and this construct will be helpful? scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 209) <https://reviews.apache.org/r/39632/#comment174065> We have a null check here and then we also have a null condition in createDataPredicate, this makes me wonder when is that branch executed? - Ajay Yadava On Jan. 7, 2016, 11:52 a.m., pavan kumar kolamuri wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39632/ > ----------------------------------------------------------- > > (Updated Jan. 7, 2016, 11:52 a.m.) > > > Review request for Falcon. > > > Bugs: https://issues.apache.org/jira/browse/FALCON-1230 > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-1230 > > > Repository: falcon-git > > > Description > ------- > > Data Availability Service for Falcon . It will notify When Data paths are > available. > > > Diffs > ----- > > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java > 72e5558 > > scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java > 1036339 > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java > 7ffb351 > > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java > 8393de0 > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java c7b4f12 > > scheduler/src/test/java/org/apache/falcon/execution/FalconExecutionServiceTest.java > d66972c > > scheduler/src/test/java/org/apache/falcon/notification/service/DataAvailabilityServiceTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/39632/diff/ > > > Testing > ------- > > Unit Tests are written > > > Thanks, > > pavan kumar kolamuri > >
