----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39632/#review112285 -----------------------------------------------------------
scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 132) <https://reviews.apache.org/r/39632/#comment172701> I am not clear on how the mapping from a Feed path to actual locations for a data window works here. For example, lets say the feed defines hourly data, /projects/falcon/test/data/{YEAR}/{MONTH}/{DAY}/{HOUR} The data window for the process is last 24 hours. This means it will have to wait for specific instances such as /projects/falcon/test/data/2015/12/30/{00..23} Here we are just passing "/projects/falcon/test/data/{YEAR}/{MONTH}/{DAY}/{HOUR}". right? scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 141) <https://reviews.apache.org/r/39632/#comment172681> This warrants a JIRA scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 148) <https://reviews.apache.org/r/39632/#comment172684> The paths have already been supplied to the requestBuilder. So, the request built by the builder should already have locations set. Using setter right after building violates the builder pattern. If that is not the case, the Builder code might require change. scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java (line 149) <https://reviews.apache.org/r/39632/#comment172686> All mandatory parameters must be supplied to the builders and there should be no setters for those. See AlarmRequestBuilder for an example. scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 54) <https://reviews.apache.org/r/39632/#comment172690> Nit : You can knock this todo also off. scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 59) <https://reviews.apache.org/r/39632/#comment172689> Nit : NUM_THREADS_PROP may be a better name? scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 64) <https://reviews.apache.org/r/39632/#comment172691> why does it need to be a List<NotificationHandler> ? For an instanceID, there is only one handler, isn't it? scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 151) <https://reviews.apache.org/r/39632/#comment172693> Use of isInterrupted rather than while(true). scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 155) <https://reviews.apache.org/r/39632/#comment172694> When the thread is interrupted, it is supposed to stop, rather than continue. AlertRerunConsumer uses a simple error handling with retries. May be you can use the same logic here. scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 168) <https://reviews.apache.org/r/39632/#comment172703> Can't we just store the pollStartTime? Then, timeout check will be : currentTime - pollStartTime > timeout scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 176) <https://reviews.apache.org/r/39632/#comment172705> Nit : Previously put is used. Just so it is easy to read, we must use the same method (may be offer is more intuitive in both places). scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 189) <https://reviews.apache.org/r/39632/#comment172692> The request can be ignored in the Event consumer itself, saves some computation. scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 38) <https://reviews.apache.org/r/39632/#comment172698> Nit : Please remove this scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 95) <https://reviews.apache.org/r/39632/#comment172706> As mentioned above, please separate out the variables into those that are mandatory and the ones that are optional. All the mandatory params should go into constructor and must be used by the builder. Similar to AlarmRequest scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 103) <https://reviews.apache.org/r/39632/#comment172709> As mentioned above, you can use pollStartTime. That way, this setter won't be required. scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 115) <https://reviews.apache.org/r/39632/#comment172710> getTimeOutInMillis seems more readable. scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 168) <https://reviews.apache.org/r/39632/#comment172711> isFirst may not be required, if acceesTime is initialized to pollStartTime. - Pallavi Rao On Dec. 26, 2015, 6:52 a.m., pavan kumar kolamuri wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39632/ > ----------------------------------------------------------- > > (Updated Dec. 26, 2015, 6: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 > >
