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