falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sowmya Ramesh" <sram...@hortonworks.com>
Subject Re: Review Request 38465: FALCON-1459 : Ability to import from database
Date Fri, 25 Sep 2015 01:07:37 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38465/#review100491
-----------------------------------------------------------



common/src/main/java/org/apache/falcon/entity/DatasourceHelper.java (line 55)
<https://reviews.apache.org/r/38465/#comment157696>

    feedCluster.getImport() can return null as import minoccurs is 0?



common/src/main/java/org/apache/falcon/entity/parser/DatasourceEntityParser.java (line 66)
<https://reviews.apache.org/r/38465/#comment157699>

    minot nit: Can these validate methods be static if possible?



common/src/main/java/org/apache/falcon/entity/parser/DatasourceEntityParser.java (line 71)
<https://reviews.apache.org/r/38465/#comment157700>

    minot nit: use StringUtils.isNotBlank()?



common/src/main/java/org/apache/falcon/entity/parser/DatasourceEntityParser.java (line 72)
<https://reviews.apache.org/r/38465/#comment157703>

    For logging instead of using string.format we use use {}. Please fix all the logging.
    
    e.g.:
    
    LOG.info("The new entry is {}.", entry);



common/src/main/java/org/apache/falcon/metadata/EntityRelationshipGraphBuilder.java (line
68)
<https://reviews.apache.org/r/38465/#comment157739>

    I don't see the code changes for adding metadata for instances. Please add it.



common/src/main/java/org/apache/falcon/util/HdfsClassLoader.java (line 49)
<https://reviews.apache.org/r/38465/#comment157726>

    Please use {} instead of + for logging. 
    
    e.g.:
    
    Instead of logger.debug("The new entry is "+entry+".");
    use
    logger.debug("The new entry is {}.", entry);
    
    second form will outperform the first form by a factor of at least 30, in case of a disabled
logging statement.
    http://www.slf4j.org/faq.html#logging_performance



common/src/main/java/org/apache/falcon/util/HdfsClassLoader.java (line 126)
<https://reviews.apache.org/r/38465/#comment157727>

    Minor nit: use StringUtils.isNotBlank?



common/src/main/java/org/apache/falcon/util/HdfsClassLoader.java (line 129)
<https://reviews.apache.org/r/38465/#comment157728>

    Instead of hard coding as / can we use new pth(parent, child) method? Just to make sure
it works on all OS'es.



common/src/main/java/org/apache/falcon/util/HdfsClassLoader.java (line 134)
<https://reviews.apache.org/r/38465/#comment157729>

    can fileURL be null?



common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java (line 76)
<https://reviews.apache.org/r/38465/#comment157730>

    Instead of adding new type cant we reuse GENEARTE as import also generates feeds unless
we save the operation type somewhere and can be useful to deintify the operation.



falcon-regression/merlin-core/src/main/java/org/apache/falcon/regression/core/util/HiveAssert.java
(line 251)
<https://reviews.apache.org/r/38465/#comment157731>

    I think this was not intended?



oozie/src/main/java/org/apache/falcon/oozie/DatabaseImportWorkflowBuilder.java (line 57)
<https://reviews.apache.org/r/38465/#comment157733>

    Have we tested this on secure cluster setup? For cases when workflow runs on diff m/c
and hdfs is on diff node, do we have to pass any creds?



oozie/src/main/java/org/apache/falcon/oozie/DatabaseImportWorkflowBuilder.java (line 147)
<https://reviews.apache.org/r/38465/#comment157734>

    can exrtaArgs be null?



oozie/src/main/java/org/apache/falcon/oozie/FeedImportCoordinatorBuilder.java (line 149)
<https://reviews.apache.org/r/38465/#comment157735>

    Minor nit: use isNotBlank



oozie/src/main/java/org/apache/falcon/oozie/FeedImportCoordinatorBuilder.java (line 162)
<https://reviews.apache.org/r/38465/#comment157736>

    nit: use StringUtils.isEmptyNotBlank



oozie/src/main/java/org/apache/falcon/oozie/ImportWorkflowBuilder.java (lines 62 - 66)
<https://reviews.apache.org/r/38465/#comment157737>

    Please use the strings from WorkflowExecutionArgs instead of hardcoding



oozie/src/main/java/org/apache/falcon/oozie/ImportWorkflowBuilder.java (line 63)
<https://reviews.apache.org/r/38465/#comment157738>

    As import geanerate feed should this be set to Ignore?


- Sowmya Ramesh


On Sept. 23, 2015, 11:58 p.m., Venkatesan Ramachandran wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38465/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2015, 11:58 p.m.)
> 
> 
> Review request for Falcon, Ajay Yadava, Balu Vellanki, Peeyush Bishnoi, Sowmya Ramesh,
and Venkat Ranganathan.
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> FALCON-1459 : Ability to import from database
> 
> 
> Diffs
> -----
> 
>   client/src/main/java/org/apache/falcon/LifeCycle.java 58a2a6c 
>   client/src/main/java/org/apache/falcon/Tag.java beeb812 
>   client/src/main/java/org/apache/falcon/entity/v0/EntityType.java 0657124 
>   client/src/main/java/org/apache/falcon/metadata/RelationshipType.java f034772 
>   client/src/main/resources/datasource-0.1.xsd PRE-CREATION 
>   client/src/main/resources/feed-0.1.xsd 2af28d2 
>   client/src/main/resources/jaxb-binding.xjb 6f1d6c7 
>   client/src/main/resources/mysql_database.xml PRE-CREATION 
>   common/src/main/java/org/apache/falcon/entity/DatasourceHelper.java PRE-CREATION 
>   common/src/main/java/org/apache/falcon/entity/EntityUtil.java ad41674 
>   common/src/main/java/org/apache/falcon/entity/FeedHelper.java 572923b 
>   common/src/main/java/org/apache/falcon/entity/parser/DatasourceEntityParser.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/entity/parser/EntityParserFactory.java 5a33201

>   common/src/main/java/org/apache/falcon/entity/parser/FeedEntityParser.java 4f5599e

>   common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java e27187b

>   common/src/main/java/org/apache/falcon/entity/v0/EntityGraph.java bd4c6cf 
>   common/src/main/java/org/apache/falcon/entity/v0/EntityIntegrityChecker.java bd32852

>   common/src/main/java/org/apache/falcon/metadata/EntityRelationshipGraphBuilder.java
8c3876c 
>   common/src/main/java/org/apache/falcon/util/HdfsClassLoader.java PRE-CREATION 
>   common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java 4454239

>   common/src/test/java/org/apache/falcon/entity/AbstractTestBase.java e9946c4 
>   common/src/test/java/org/apache/falcon/entity/EntityTypeTest.java 640e87d 
>   common/src/test/java/org/apache/falcon/entity/FeedHelperTest.java c70cfcc 
>   common/src/test/java/org/apache/falcon/entity/parser/DatasourceEntityParserTest.java
PRE-CREATION 
>   common/src/test/java/org/apache/falcon/entity/parser/FeedEntityParserTest.java b6fdb13

>   common/src/test/java/org/apache/falcon/entity/v0/EntityGraphTest.java 3863b11 
>   common/src/test/resources/config/datasource/datasource-0.1.xml PRE-CREATION 
>   common/src/test/resources/config/datasource/datasource-invalid-0.1.xml PRE-CREATION

>   common/src/test/resources/config/feed/feed-import-0.1.xml PRE-CREATION 
>   common/src/test/resources/config/feed/feed-import-invalid-0.1.xml PRE-CREATION 
>   docs/src/site/twiki/EntitySpecification.twiki d4f4140 
>   falcon-regression/merlin-core/src/main/java/org/apache/falcon/regression/core/util/HiveAssert.java
2a934b5 
>   oozie/src/main/java/org/apache/falcon/oozie/DatabaseImportWorkflowBuilder.java PRE-CREATION

>   oozie/src/main/java/org/apache/falcon/oozie/FeedImportCoordinatorBuilder.java PRE-CREATION

>   oozie/src/main/java/org/apache/falcon/oozie/ImportWorkflowBuilder.java PRE-CREATION

>   oozie/src/main/java/org/apache/falcon/oozie/OozieCoordinatorBuilder.java a04ae95 
>   oozie/src/main/java/org/apache/falcon/oozie/OozieOrchestrationWorkflowBuilder.java
3213a70 
>   oozie/src/main/java/org/apache/falcon/oozie/feed/FeedBundleBuilder.java b819dee 
>   oozie/src/main/resources/action/feed/import-sqoop-database-action.xml PRE-CREATION

>   pom.xml 646de69 
>   webapp/pom.xml 828f7f5 
>   webapp/src/test/java/org/apache/falcon/lifecycle/FeedImportIT.java PRE-CREATION 
>   webapp/src/test/java/org/apache/falcon/resource/TestContext.java d067dee 
>   webapp/src/test/java/org/apache/falcon/util/HsqldbTestUtils.java PRE-CREATION 
>   webapp/src/test/resources/datasource-template.xml PRE-CREATION 
>   webapp/src/test/resources/feed-template3.xml PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38465/diff/
> 
> 
> Testing
> -------
> 
> * Unit tests
> * Integration tests
> * Manual tests
>   * Setup MySQL, create table and populate
>   * Create datasource and feed entity with import policy in Falcon  
>   * Made sure the data lands up in the HDFS.
> 
> 
> Thanks,
> 
> Venkatesan Ramachandran
> 
>


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