----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32688/#review78368 -----------------------------------------------------------
common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java <https://reviews.apache.org/r/32688/#comment126930> 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. common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java <https://reviews.apache.org/r/32688/#comment126931> Excessive logging ? DEBUG level ? common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java <https://reviews.apache.org/r/32688/#comment126933> 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. common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java <https://reviews.apache.org/r/32688/#comment126934> Not a fan of fall through case statement. Agreed that this is not as bad as each of the case label having some code in them. common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java <https://reviews.apache.org/r/32688/#comment126936> Is the CurrentUser authenticated ? What user are we proxying as ? common/src/main/java/org/apache/falcon/catalog/CatalogPartitionHandler.java <https://reviews.apache.org/r/32688/#comment126937> Understand that this will handle the cases of re-run where data is re-added. In that scenario, if drop partitions are complete, but add partition where to fail mid way, would it leave the table in a bad state ? This might be a large gap, if this isn't atomic. If this is indeed a valid issue, we need to address this. But given the largish nature of the fix, we can file another JIRA and handle this separately. common/src/main/java/org/apache/falcon/catalog/HiveCatalogService.java <https://reviews.apache.org/r/32688/#comment126938> debug level ? common/src/main/java/org/apache/falcon/catalog/HiveCatalogService.java <https://reviews.apache.org/r/32688/#comment126939> Want to be bit conservative when it comes to logging at info level common/src/main/java/org/apache/falcon/entity/CatalogStorage.java <https://reviews.apache.org/r/32688/#comment126940> Need to expand imports. common/src/main/java/org/apache/falcon/expression/ExpressionHelper.java <https://reviews.apache.org/r/32688/#comment126942> Should this be final ? - Srikanth Sundarrajan 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 > >
