falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "pavan kumar kolamuri" <pavan.kolam...@gmail.com>
Subject Re: Review Request 39632: Data Availability Service for Falcon
Date Wed, 30 Dec 2015 10:39:55 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message