> On March 31, 2015, 4:36 p.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java, > > line 84 > > <https://reviews.apache.org/r/32688/diff/1/?file=911160#file911160line84> > > > > This log line will appear too many times. does it makes sense we log > > this inside a function where the IS_CATALOG_ENABLED is set.
Agreed. Makes sense to Log this at falcon start-up > On March 31, 2015, 4:36 p.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java, > > line 95 > > <https://reviews.apache.org/r/32688/diff/1/?file=911160#file911160line95> > > > > Excessive logging ? DEBUG level ? I think this would useful at info level. Ideally this scenario shouldn't happen. If it happens, the logs would show abrupt exit leaving the user confused abt the exact error. > On March 31, 2015, 4:36 p.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java, > > line 110 > > <https://reviews.apache.org/r/32688/diff/1/?file=911160#file911160line110> > > > > Nit. getCatalogStorage seems to mislead reader into assuming that the > > feed is catalog based, while in fact if it is catalog based, it should be > > NOOP. Please rename this to getCatalogStorageFromFeedProperties to indicate > > that we are using optional properties to build the catalog storage object > > and the that the feed isn't natively catalog based. definitely makes sense to rename it. would be helpful > On March 31, 2015, 4:36 p.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java, > > line 164 > > <https://reviews.apache.org/r/32688/diff/1/?file=911160#file911160line164> > > > > Is the CurrentUser authenticated ? What user are we proxying as ? There is no current user. We would be proxying as the user that started falcon. > On March 31, 2015, 4:36 p.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/catalog/HiveCatalogService.java, > > line 242 > > <https://reviews.apache.org/r/32688/diff/1/?file=911161#file911161line242> > > > > debug level ? I believe this would be helpful at info level. > On March 31, 2015, 4:36 p.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/catalog/HiveCatalogService.java, > > line 250 > > <https://reviews.apache.org/r/32688/diff/1/?file=911161#file911161line250> > > > > Want to be bit conservative when it comes to logging at info level Changed to debug > On March 31, 2015, 4:36 p.m., Srikanth Sundarrajan wrote: > > common/src/main/java/org/apache/falcon/entity/CatalogStorage.java, line 47 > > <https://reviews.apache.org/r/32688/diff/1/?file=911162#file911162line47> > > > > Need to expand imports. Taken care - Suhas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32688/#review78368 ----------------------------------------------------------- On March 31, 2015, 11:35 a.m., Srikanth Sundarrajan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32688/ > ----------------------------------------------------------- > > (Updated March 31, 2015, 11:35 a.m.) > > > Review request for Falcon. > > > Bugs: FALCON-1091 > https://issues.apache.org/jira/browse/FALCON-1091 > > > Repository: falcon-git > > > Description > ------- > > FALCON-1091 Monitoring plugin code > > > Diffs > ----- > > common/src/main/java/org/apache/falcon/catalog/AbstractCatalogService.java > 9abdc93 > common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java > PRE-CREATION > common/src/main/java/org/apache/falcon/catalog/HiveCatalogService.java > 25a4a46 > common/src/main/java/org/apache/falcon/entity/CatalogStorage.java 59f558b > common/src/main/java/org/apache/falcon/entity/FeedHelper.java ca31f95 > common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java > 1ba7b9d > common/src/main/java/org/apache/falcon/entity/common/FeedDataPath.java > 6ededbb > common/src/main/java/org/apache/falcon/expression/ExpressionHelper.java > 33ec59c > common/src/main/java/org/apache/falcon/util/FalconRadixUtils.java 4bf6e00 > > common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java > 8d69b9a > common/src/main/resources/startup.properties 99dab59 > common/src/test/java/org/apache/falcon/entity/FeedDataPathTest.java c405556 > common/src/test/java/org/apache/falcon/entity/FeedHelperTest.java f6994fc > common/src/test/java/org/apache/falcon/entity/FileSystemStorageTest.java > 1667161 > > hadoop-dependencies/src/versioned-src/v2/java/org/apache/hadoop/mapred/ClassicClientProtocolProvider.java > 2167375 > oozie/src/main/java/org/apache/falcon/logging/LogProvider.java 2e5dffb > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionCoordinatorBuilder.java > 7a87919 > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java > 462e26b > > oozie/src/test/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilderTest.java > 545beb1 > prism/pom.xml 4a3054a > retention/src/test/java/org/apache/falcon/retention/FeedEvictorTest.java > 970d381 > test-tools/hadoop-webapp/src/main/resources/mapred-site.xml cf297de > test-tools/hadoop-webapp/src/main/resources/yarn-site.xml 658752b > > webapp/src/test/java/org/apache/falcon/catalog/CatalogPartitionHandlerIT.java > PRE-CREATION > webapp/src/test/java/org/apache/falcon/catalog/HiveCatalogServiceIT.java > 71616e9 > > webapp/src/test/java/org/apache/falcon/lifecycle/TableStorageFeedEvictorIT.java > 6982b65 > webapp/src/test/java/org/apache/falcon/util/HiveTestUtils.java 19274b9 > webapp/src/test/java/org/apache/falcon/util/OozieTestUtils.java e67fe2a > webapp/src/test/resources/cluster-template.xml 16b7c8c > webapp/src/test/resources/feed-template1.xml 456f7ce > webapp/src/test/resources/feed-template2.xml d4901fa > > Diff: https://reviews.apache.org/r/32688/diff/ > > > Testing > ------- > > > Thanks, > > Srikanth Sundarrajan > >
