You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by pavan kumar kolamuri <pa...@gmail.com> on 2015/12/21 15:28:48 UTC
Re: Review Request 39632: Data Availability Service for Falcon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39632/
-----------------------------------------------------------
(Updated Dec. 21, 2015, 2:28 p.m.)
Review request for Falcon.
Changes
-------
Rebased the patch
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 (updated)
-----
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/notification/service/util/GlobUtil.java PRE-CREATION
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
Re: Review Request 39632: Data Availability Service for Falcon
Posted by pavan kumar kolamuri <pa...@gmail.com>.
> 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.
>
> pavan kumar kolamuri wrote:
> Will think of using poll start time and update
When building this service even i thought of doing something w.r.t to poll start time, but in that approach delay time in queue also comes into effect while checking request until timeout. Suppose request has 10 secs timeout and polling frequency 2 secs, it might be in queue for 2 secs if queue is busy, then we end up delaying one more time since we thought it came second. And also with this approach there is chance that request might not check for 5 times eventhough polling frequency is 2 secs and timeout is 10 secs. With the current approach this was taken using queuetime.
> 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.
>
> pavan kumar kolamuri wrote:
> Ok will check and update
Explained the differences between approaches in previous comments.
> On Dec. 30, 2015, 7:32 a.m., Pallavi Rao wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 189
> > <https://reviews.apache.org/r/39632/diff/3/?file=1176408#file1176408line189>
> >
> > The request can be ignored in the Event consumer itself, saves some computation.
Yes make sense will do this as well.
- 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
>
>
Re: Review Request 39632: Data Availability Service for Falcon
Posted by pavan kumar kolamuri <pa...@gmail.com>.
> 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
>
>
Re: Review Request 39632: Data Availability Service for Falcon
Posted by Pallavi Rao <pa...@inmobi.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 39632: Data Availability Service for Falcon
Posted by pavan kumar kolamuri <pa...@gmail.com>.
> On Jan. 6, 2016, 12:32 p.m., Pallavi Rao wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 166
> > <https://reviews.apache.org/r/39632/diff/4/?file=1180737#file1180737line166>
> >
> > As discussed dets delegate this to Request.hasTimeout() method. The DataAvailabilityService shouldn't worry about this logic.
Yes will fix
> On Jan. 6, 2016, 12:32 p.m., Pallavi Rao wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 171
> > <https://reviews.apache.org/r/39632/diff/4/?file=1180737#file1180737line171>
> >
> > As discussed dets delegate this to Request.accessed() method. The DataAvailabilityService shouldn't worry about this logic.
ok sure will fix
> On Jan. 6, 2016, 12:32 p.m., Pallavi Rao wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, line 78
> > <https://reviews.apache.org/r/39632/diff/4/?file=1180738#file1180738line78>
> >
> > What happens during Falcon restarts, since we are not polling how long we have polled. The polling will start all over again and an instance which was supposed to, lets say, time out at 2nd hour (from instance time), might time-out much later than that.
As of know its only taking from created time , will think about this in next patch along with refactoring of DataNotification Request
- pavan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39632/#review113057
-----------------------------------------------------------
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
>
>
Re: Review Request 39632: Data Availability Service for Falcon
Posted by Pallavi Rao <pa...@inmobi.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39632/#review113057
-----------------------------------------------------------
scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 165)
<https://reviews.apache.org/r/39632/#comment173565>
As discussed dets delegate this to Request.hasTimeout() method. The DataAvailabilityService shouldn't worry about this logic.
scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 170)
<https://reviews.apache.org/r/39632/#comment173566>
As discussed dets delegate this to Request.accessed() method. The DataAvailabilityService shouldn't worry about this logic.
scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 69)
<https://reviews.apache.org/r/39632/#comment173568>
What happens during Falcon restarts, since we are not polling how long we have polled. The polling will start all over again and an instance which was supposed to, lets say, time out at 2nd hour (from instance time), might time-out much later than that.
- Pallavi Rao
On Jan. 4, 2016, 10:21 a.m., pavan kumar kolamuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39632/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2016, 10:21 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
>
>
Re: Review Request 39632: Data Availability Service for Falcon
Posted by pavan kumar kolamuri <pa...@gmail.com>.
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 65
> > <https://reviews.apache.org/r/39632/diff/1/?file=1107368#file1107368line65>
> >
> > Can you please document in detail the purpose of this field?
sure will add
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 71
> > <https://reviews.apache.org/r/39632/diff/1/?file=1107368#file1107368line71>
> >
> > 2 similar logs in 2 lines, will merging them to one suffice?
ok will remove one
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, line 140
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186209#file1186209line140>
> >
> > Shouldn't this TODO be removed now? Is there still something left?
No still lot has to be changed here it will be part of EL expressions until then we can leave this
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java, line 141
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186209#file1186209line141>
> >
> > What change in polling frequency is required and why? Any reason to not include that in this JIRA itself?
As of now polling Frequency is feed frequency or random value which is not correct. I am fixing this in next jira for EL Expressions
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java, line 33
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186210#file1186210line33>
> >
> > nit: change to plural "dataLocations"
ok
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java, line 43
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186210#file1186210line43>
> >
> > When is a data event invalid? It will be useful to put a comment here as this status is slightly unintuitive for the uninitiated.
thought of removing in next patch as it is already done. Removed now
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java, line 62
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186210#file1186210line62>
> >
> > nit: convert to plural getDataLocations
ok
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java, line 66
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186210#file1186210line66>
> >
> > nit: Change to plural "setDataLocations" and "locations"
ok
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java, line 74
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186210#file1186210line74>
> >
> > Isn't this always fixed to DATA?
No, Location type can be Meta also , where we have search for metadata paths. As of now we can leave it we will get full clarity once we implement EL Expressions Reslover and start jobs based on data availability
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 64
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line64>
> >
> > 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?
Added a comment, whenever event is unregistered it is added here
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 65
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line65>
> >
> > 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?
Tracked in FALCON-1737
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 70
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line70>
> >
> > excessive logging?
removed
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 156
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line156>
> >
> > 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
Yes will do
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 192
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line192>
> >
> > Please raise a JIRA for this and delete the TODO. Otherwise no one else knows about the missing capabilities.
Ok will raise a jira, until that its better to keep it here also
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 197
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line197>
> >
> > What will be callbackID here? Can two different requests have same CallbackID?
Yes possible, when request send by two different handlers. Will remove it if not required when adding integration tests for Databased triggering
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 219
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line219>
> >
> > nit: getUnavailablePaths (I hate to point grammar mistakes but some time back I realized that the code looks much better after fixing these)
ok will do
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 221
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line221>
> >
> > nit: areAllPathsPresent or allPathsExist will be more appropriate IMO
ok
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 222
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line222>
> >
> > won't "return areAllPathsExist(locations)" suffice?
ok
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 225
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line225>
> >
> > nit: Don't concatenate the exception object.
> >
> > LOG.error("message", e) should be just fine.
Removed Exception from message
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 227
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line227>
> >
> > nit: don't concatenate the exception
Didn't get this exactly
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 244
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186211#file1186211line244>
> >
> > Casting to boolean shouldn't be required here. May be you need to parameterize pathInfo variable.
Will change
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, line 41
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186212#file1186212line41>
> >
> > What is the boolean for? docs please
Added
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, line 45
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186212#file1186212line45>
> >
> > nit: isn't this always in Millis and should be accessTimeInMillis as per naming convention of other variables.
Changed
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, line 47
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186212#file1186212line47>
> >
> > I think the datanotification service should be independent of the locationType and this shouldn't be required.
Explained in pervious comments
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, line 48
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186212#file1186212line48>
> >
> > what does isFirst represent? docs please
added
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, line 79
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186212#file1186212line79>
> >
> > nit: you can do createdTimeInMillis = accessTime.
ok
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, line 99
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186212#file1186212line99>
> >
> > Isn't this an internal detail and hence should be private?
Its accessed from DataAvailabilityService so it should be public
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, line 138
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186212#file1186212line138>
> >
> > Why is a setter method returning this?
removed
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, line 158
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186212#file1186212line158>
> >
> > nit: same doc for getLocations & getLocationMap??
Fixed
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 167
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186213#file1186213line167>
> >
> > Should the DataPredicate be concerned about locationType?
As i said locationtype can be data and meta also
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 169
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186213#file1186213line169>
> >
> > I am curious, under what circumstances one will create a data predicate with null paths and this construct will be helpful?
No it cant be. Fixed
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 209
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186213#file1186213line209>
> >
> > 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?
Fixed
> On Jan. 8, 2016, 8:24 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java, line 171
> > <https://reviews.apache.org/r/39632/diff/5/?file=1186212#file1186212line171>
> >
> > Won't returning the difference suffice here?
I hope its ok to leave like this
- pavan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39632/#review106556
-----------------------------------------------------------
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
>
>
Re: Review Request 39632: Data Availability Service for Falcon
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 39632: Data Availability Service for Falcon
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39632/#review113699
-----------------------------------------------------------
Ship it!
Ship It!
- Ajay Yadava
On Jan. 11, 2016, 7:23 a.m., pavan kumar kolamuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39632/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 7:23 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
>
>
Re: Review Request 39632: Data Availability Service for Falcon
Posted by pavan kumar kolamuri <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39632/
-----------------------------------------------------------
(Updated Jan. 11, 2016, 7:23 a.m.)
Review request for Falcon.
Changes
-------
Addressed comments
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 (updated)
-----
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
Re: Review Request 39632: Data Availability Service for Falcon
Posted by pavan kumar kolamuri <pa...@gmail.com>.
> On Jan. 8, 2016, 3:29 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 116
> > <https://reviews.apache.org/r/39632/diff/6/?file=1187407#file1187407line116>
> >
> > I think Data Service shouldn't care which location type it is and this should be removed.
Removed
> On Jan. 8, 2016, 3:29 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 217
> > <https://reviews.apache.org/r/39632/diff/6/?file=1187407#file1187407line217>
> >
> > I believe this is a typo and should be a comma instead of "+"
Fixed
> On Jan. 8, 2016, 3:29 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java, line 229
> > <https://reviews.apache.org/r/39632/diff/6/?file=1187407#file1187407line229>
> >
> > IMHO we don't have a usecase for handlers currently and we should avoid adding it until we have one.
Ok will do
> On Jan. 8, 2016, 3:29 p.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java, line 169
> > <https://reviews.apache.org/r/39632/diff/6/?file=1187409#file1187409line169>
> >
> > We shouldn't accept null paths & locations.
Done
- pavan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39632/#review113478
-----------------------------------------------------------
On Jan. 8, 2016, 12:30 p.m., pavan kumar kolamuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39632/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2016, 12:30 p.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
>
>
Re: Review Request 39632: Data Availability Service for Falcon
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39632/#review113478
-----------------------------------------------------------
scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 115)
<https://reviews.apache.org/r/39632/#comment174213>
I think Data Service shouldn't care which location type it is and this should be removed.
scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 216)
<https://reviews.apache.org/r/39632/#comment174216>
I believe this is a typo and should be a comma instead of "+"
scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataAvailabilityService.java (line 228)
<https://reviews.apache.org/r/39632/#comment174219>
IMHO we don't have a usecase for handlers currently and we should avoid adding it until we have one.
scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java (line 169)
<https://reviews.apache.org/r/39632/#comment174211>
We shouldn't accept null paths & locations.
- Ajay Yadava
On Jan. 8, 2016, 12:30 p.m., pavan kumar kolamuri wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39632/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2016, 12:30 p.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
>
>
Re: Review Request 39632: Data Availability Service for Falcon
Posted by pavan kumar kolamuri <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39632/
-----------------------------------------------------------
(Updated Jan. 8, 2016, 12:30 p.m.)
Review request for Falcon.
Changes
-------
Addressed comments
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 (updated)
-----
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
Re: Review Request 39632: Data Availability Service for Falcon
Posted by pavan kumar kolamuri <pa...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Addressed comments
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 (updated)
-----
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
Re: Review Request 39632: Data Availability Service for Falcon
Posted by pavan kumar kolamuri <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39632/
-----------------------------------------------------------
(Updated Jan. 4, 2016, 10:21 a.m.)
Review request for Falcon.
Changes
-------
Adressed comments and added synchronization in few places
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 (updated)
-----
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
Re: Review Request 39632: Data Availability Service for Falcon
Posted by pavan kumar kolamuri <pa...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Updated Patch by removing glob pattern
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 (updated)
-----
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