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 26096: FALCON-301 Disallow feeds with same location
Date Tue, 30 Sep 2014 09:55:13 GMT


> On Sept. 30, 2014, 7:06 a.m., Suma Shivaprasad wrote:
> > common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java, line
59
> > <https://reviews.apache.org/r/26096/diff/1/?file=706326#file706326line59>
> >
> >     should we throw an error if EntityType != FEED is passed?

IMHO we shouldn't, other listeners might be interested in this event.


> On Sept. 30, 2014, 7:06 a.m., Suma Shivaprasad wrote:
> > common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java, line
64
> > <https://reviews.apache.org/r/26096/diff/1/?file=706326#file706326line64>
> >
> >     could use StringUtils from apache-commons

Does it provide more funcitonality than this for isEmpty()?


> On Sept. 30, 2014, 7:06 a.m., Suma Shivaprasad wrote:
> > common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java, line
66
> > <https://reviews.apache.org/r/26096/diff/1/?file=706326#file706326line66>
> >
> >     same as above. could use StringUtils.trim..which removes ctrl chars and whitespace

Will switch for this case.


> On Sept. 30, 2014, 7:06 a.m., Suma Shivaprasad wrote:
> > common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java, line
90
> > <https://reviews.apache.org/r/26096/diff/1/?file=706326#file706326line90>
> >
> >     will one of the delete fails cause the entire feed to be aborted? Instead could
we catch the Exception and log the error cases

I think catching exception and suppressing will be wrong here. It will fail in cases like
where you delete a feed and one of locations failed to delete and then you try to resubmit
it, falcon will not let you submit it and there is no other way to delete it.


> On Sept. 30, 2014, 7:06 a.m., Suma Shivaprasad wrote:
> > common/src/main/java/org/apache/falcon/util/RadixTree.java, line 167
> > <https://reviews.apache.org/r/26096/diff/1/?file=706330#file706330line167>
> >
> >     could remove extra blank lines

Will do.


- Ajay


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26096/#review54956
-----------------------------------------------------------


On Sept. 26, 2014, 8:34 p.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26096/
> -----------------------------------------------------------
> 
> (Updated Sept. 26, 2014, 8:34 p.m.)
> 
> 
> Review request for Falcon and Srikanth Sundarrajan.
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> FALCON-301 Disallow feeds with same location
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/entity/store/LocationStore.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/util/KeyAlreadyExistsException.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/util/RadixNode.java PRE-CREATION 
>   common/src/main/java/org/apache/falcon/util/RadixTree.java PRE-CREATION 
>   common/src/main/resources/startup.properties e233b2a 
>   common/src/test/java/org/apache/falcon/entity/AbstractTestBase.java 2140335 
>   common/src/test/java/org/apache/falcon/entity/store/FeedLocationStoreTest.java PRE-CREATION

>   common/src/test/java/org/apache/falcon/group/FeedGroupMapTest.java a6c52e3 
>   common/src/test/java/org/apache/falcon/util/RadixNodeTest.java PRE-CREATION 
>   common/src/test/java/org/apache/falcon/util/RadixTreeTest.java PRE-CREATION 
>   src/conf/startup.properties 78466af 
>   webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java 0943103 
>   webapp/src/test/java/org/apache/falcon/cli/FalconCLISmokeIT.java cb0dd2d 
>   webapp/src/test/java/org/apache/falcon/process/PigProcessIT.java 0f2a971 
>   webapp/src/test/java/org/apache/falcon/process/TableStorageProcessIT.java 51afbb8 
>   webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseyIT.java ed70a0b

>   webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseySmokeIT.java d4a1d8a

>   webapp/src/test/java/org/apache/falcon/resource/EntityManagerPaginationJerseyIT.java
bd68e57 
>   webapp/src/test/java/org/apache/falcon/resource/MetadataResourceJerseyIT.java 5249888

>   webapp/src/test/java/org/apache/falcon/resource/TestContext.java e9545d1 
>   webapp/src/test/resources/feed-template1.xml 456f7ce 
>   webapp/src/test/resources/feed-template2.xml d4901fa 
> 
> Diff: https://reviews.apache.org/r/26096/diff/
> 
> 
> Testing
> -------
> 
> Yes. Unit Tests are present for new classes and all unit & integration tests pass.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>


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