falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ajay Yadava" <ajayn...@gmail.com>
Subject Re: Review Request 39632: Data Availability Service for Falcon
Date Fri, 08 Jan 2016 08:24:24 GMT

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


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