> On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 132 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176406#file1176406line132> > > > > 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?
This is not fixed as part of this jira. this is taken care as part of EL expression evaluation > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 141 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176406#file1176406line141> > > > > This warrants a JIRA Yes this will get fixed as part of subtask of https://issues.apache.org/jira/browse/FALCON-1435 > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 148 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176406#file1176406line148> > > > > 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. Yes i understand this , i am fixing all these as part of https://issues.apache.org/jira/browse/FALCON-1435 sub tasks . We need to set cluster also as mandatory , that should also be part of builder > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, > > line 149 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176406#file1176406line149> > > > > All mandatory parameters must be supplied to the builders and there > > should be no setters for those. See AlarmRequestBuilder for an example. Yes understand this, i am cleaning processExecutionInstance completely as part of input paths evaluation and will set all mandatory params as part of that > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, > > line 54 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176408#file1176408line54> > > > > Nit : You can knock this todo also off. Sure will remove > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, > > line 59 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176408#file1176408line59> > > > > Nit : NUM_THREADS_PROP may be a better name? Sure will rename > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, > > line 64 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176408#file1176408line64> > > > > why does it need to be a List<NotificationHandler> ? For an instanceID, > > there is only one handler, isn't it? We thought Datavailabilty can be used for SLA monitoring also, thats why added list for listeners > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, > > line 151 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176408#file1176408line151> > > > > Use of isInterrupted rather than while(true). ok sure > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, > > line 155 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176408#file1176408line155> > > > > 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. Ok sure, but we have delayqueue has some issues, it will stop datavailabilty service right is it ok ? > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, > > line 168 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176408#file1176408line168> > > > > Can't we just store the pollStartTime? Then, timeout check will be : > > currentTime - pollStartTime > timeout Both represents same. I think this is ok to store queuetime. Am i missing something ? > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, > > line 176 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176408#file1176408line176> > > > > 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). All methods do same if i am not wrong. Will make it consistent and will use offer > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, > > line 38 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176409#file1176409line38> > > > > Nit : Please remove this sure will remove > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, > > line 103 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176409#file1176409line103> > > > > 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 This is already set in constructor , it should be modified later, i will update the exact use case while addressing it. > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, > > line 111 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176409#file1176409line111> > > > > As mentioned above, you can use pollStartTime. That way, this setter > > won't be required. Will think of using poll start time and update > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, > > line 123 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176409#file1176409line123> > > > > getTimeOutInMillis seems more readable. sure will fix > On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote: > > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, > > line 176 > > <https://reviews.apache.org/r/39632/diff/3/?file=1176409#file1176409line176> > > > > isFirst may not be required, if acceesTime is initialized to > > pollStartTime. Ok will check and update - pavan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39632/#review112285 ----------------------------------------------------------- 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 > >
