You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Srikanth Sundarrajan <sr...@hotmail.com> on 2014/10/09 13:06:04 UTC
Re: Review Request 26096: FALCON-301 Disallow feeds with same location

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



common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java
<https://reviews.apache.org/r/26096/#comment95397>

    Location is not present when the feed is based on catalog. Does it make sense to create a child jira to track the catalog related changes.



common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java
<https://reviews.apache.org/r/26096/#comment95398>

    If location object is present, then path, type etc can't be null. They are mandated in the schema definition



common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java
<https://reviews.apache.org/r/26096/#comment95399>

    Would it be simpler to invoke onRemove on old entity and onAdd on new entity instead ?



common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java
<https://reviews.apache.org/r/26096/#comment95400>

    This is invoked only on server start and can be treated similar to onAdd.



common/src/main/java/org/apache/falcon/entity/store/LocationStore.java
<https://reviews.apache.org/r/26096/#comment95401>

    Not sure if this is the correct interface for RadixTree to implement. This seems too specific for location (at least the name). I would have considered NavigableMap or SortedMap as a viable interface for RadixTree to implement. Please evaluate the possibility and weigh the options



common/src/main/java/org/apache/falcon/util/RadixNode.java
<https://reviews.apache.org/r/26096/#comment95402>

    Empty javadoc.



common/src/main/java/org/apache/falcon/util/RadixNode.java
<https://reviews.apache.org/r/26096/#comment96325>

    Does it make sense for this to be package private ?



common/src/main/java/org/apache/falcon/util/RadixNode.java
<https://reviews.apache.org/r/26096/#comment96324>

    Not a good idea to return the list directly, as that would allow mutation of the list outside of this class.



common/src/main/java/org/apache/falcon/util/RadixTree.java
<https://reviews.apache.org/r/26096/#comment95403>

    Use @Nonnull to annotate arguments which are not null and likewise use appropriate args which are nullable.


- Srikanth Sundarrajan


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