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 Fri, 08 Jan 2016 12:10:48 GMT


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


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