falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pallavi Rao" <pallavi....@inmobi.com>
Subject Re: Review Request 39632: Data Availability Service for Falcon
Date Wed, 30 Dec 2015 07:32:08 GMT

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


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