falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Suhas Vasu" <suhas....@gmail.com>
Subject Re: Review Request 32688: FALCON-1091 Monitoring plugin code
Date Wed, 01 Apr 2015 08:07:01 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message