> On March 4, 2015, 3:34 a.m., Srikanth Sundarrajan wrote: > > client/src/main/java/org/apache/falcon/cli/FalconCLI.java, line 631 > > <https://reviews.apache.org/r/31660/diff/1/?file=882633#file882633line631> > > > > One thing for us to think about is if this is applicable only for file > > system based feeds. -path switch seems to suggest that it is restricted to > > FS based feed. Also we need to see how the radix tree would hold catalog > > based instance. Catalog related work can happen in an independent jira, but > > it is important to establish this correctly as an external facing contract.
There is already an issue logged for CatalogBased feeds under the same parent JIRA. I am not familiar enough with catalog based filesystem and feeds to see if there are any elements which are not generic enough for both. > On March 4, 2015, 3:34 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java, > > line 136 > > <https://reviews.apache.org/r/31660/diff/1/?file=882636#file882636line136> > > > > Does it make sense for FeedHelper.getLocations to get > > clusterSpecificLocation always. This seems to be repeated in many places > > (outside of this patch) and might be good to cleanup a bit. Agreed. I will log a separate JIRA for the same. > On March 4, 2015, 3:34 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/util/RadixTree.java, line 331 > > <https://reviews.apache.org/r/31660/diff/1/?file=882638#file882638line331> > > > > Too much logging ? Actually, while developing I found a bug and I had to log these to debug, mostly at decision nodes. I would definitely like to have these in logs if things need debugging but I can change it to trace level if that improves things. > On March 4, 2015, 3:34 a.m., Srikanth Sundarrajan wrote: > > common/src/main/resources/log4j.xml, line 68 > > <https://reviews.apache.org/r/31660/diff/1/?file=882639#file882639line68> > > > > Why is this change needed ? Is it accidentally left over post a debug > > session ? This is a left over post a debug session. I will fix it. > On March 4, 2015, 3:34 a.m., Srikanth Sundarrajan wrote: > > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java, > > line 969 > > <https://reviews.apache.org/r/31660/diff/1/?file=882646#file882646line969> > > > > Should perhaps be "Reverse lookup not supported for this entityType" Yes, your suggestion is much better. > On March 4, 2015, 3:34 a.m., Srikanth Sundarrajan wrote: > > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java, > > line 979 > > <https://reviews.apache.org/r/31660/diff/1/?file=882646#file882646line979> > > > > Not clear what is the significance of the "/" at the end. There are two > > calls made, one with slash and one without. Can you elaborate and include > > this as code comments Thinking behind this is that /data/cas and /data/cas/ are actually same for purpose of matching, radix tree will not treat the two inputs as same so I check for it. This is FALCON-1037. > On March 4, 2015, 3:34 a.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/util/RadixTree.java, line 267 > > <https://reviews.apache.org/r/31660/diff/1/?file=882638#file882638line267> > > > > You need this at debug level or trace ? These are general events that I actually used for debugging after I found some bug. These are mostly key events/decisions taken during the delete process. But I will change it to trace. - Ajay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31660/#review75146 ----------------------------------------------------------- On March 3, 2015, 3:30 a.m., Ajay Yadava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31660/ > ----------------------------------------------------------- > > (Updated March 3, 2015, 3:30 a.m.) > > > Review request for Falcon and Srikanth Sundarrajan. > > > Bugs: https://issues.apache.org/jira/browse/FALCON-822 > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-822 > > > Repository: falcon-git > > > Description > ------- > > Falcon-822: Exposed an API over radix tree to enable users to reverse look up > the feed name using a feed instance path. Also addressed FALCON-1037 to > handle trailing slashes. First set of review comments given in JIRA are also > addressed > > > Diffs > ----- > > client/src/main/java/org/apache/falcon/ResponseHelper.java 5f36e3a > client/src/main/java/org/apache/falcon/cli/FalconCLI.java ac76a9c > client/src/main/java/org/apache/falcon/client/FalconClient.java 86397c4 > client/src/main/java/org/apache/falcon/resource/FeedLookupResult.java > PRE-CREATION > common/src/main/java/org/apache/falcon/entity/store/FeedLocationStore.java > e056d96 > common/src/main/java/org/apache/falcon/util/FalconRadixUtils.java bbd73c7 > common/src/main/java/org/apache/falcon/util/RadixTree.java 6cd79f5 > common/src/main/resources/log4j.xml 75c8267 > common/src/main/resources/startup.properties 433c2a8 > > common/src/test/java/org/apache/falcon/entity/store/FeedLocationStoreTest.java > 86ef775 > common/src/test/java/org/apache/falcon/util/RadixTreeTest.java 109c24d > docs/src/site/twiki/FalconCLI.twiki 547aa7d > docs/src/site/twiki/restapi/FeedLookup.twiki PRE-CREATION > docs/src/site/twiki/restapi/ResourceList.twiki 2f37bb3 > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java > 424bada > > prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java > 5f711ee > prism/src/test/java/org/apache/falcon/resource/EntityManagerTest.java > 470f866 > src/conf/startup.properties 2db4b1e > > webapp/src/main/java/org/apache/falcon/resource/SchedulableEntityManager.java > a83f0cf > webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java d46f112 > > Diff: https://reviews.apache.org/r/31660/diff/ > > > Testing > ------- > > Unit tests and Integration tests are added. > > ( I have also addresssed the first set of code review comments & Falcon-1037 > as well to handle trailing slashes) > > > Thanks, > > Ajay Yadava > >
