Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Sqoop >> mail # dev >> Review Request: SQOOP-749: Exports Using Stored Procedures (Functions)


Copy link to this message
-
Re: Review Request: SQOOP-749: Exports Using Stored Procedures (Functions)

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8806/#review15340
-----------------------------------------------------------
Hi Nick,
Thank you for introducing explicit PostgreSQL third party test. I think that test coverage is reasonable for newly introduced functionality - let's create follow up JIRAs to add similar tests to other third party tests once this gets committed.

I've noticed that your patch is changing order of import statements in a lot of files. Such changes are complicating development as they are introducing unnecessary changes. Would you mind putting the order back? I've highlighted all of them for you in the review notes below.

Other than than I just realized that adding new abstract methods to ConnManager is dangerous for backward compatibility - it might break already existing connectors that inherits from ConnManager and do not implement such methods. As sqoop 1 is considered stable, would you mind changing those methods to non abstract and throw an exception in case that they will be called?
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8806/#comment32992>

    Would you mind putting the import statements in original order?
    

src/java/org/apache/sqoop/manager/ConnManager.java
<https://reviews.apache.org/r/8806/#comment32990>

    Would you mind changing this abstract method to a normal method returning Exception that it's not supported?
    
    I'm concerned here with backward compatibly for connectors that are already out there.

src/java/org/apache/sqoop/manager/ConnManager.java
<https://reviews.apache.org/r/8806/#comment32991>

    Would you mind changing this abstract method to a normal method returning Exception that it's not supported?
    
    I'm concerned here with backward compatibly for connectors that are already out there.

src/java/org/apache/sqoop/manager/SqlManager.java
<https://reviews.apache.org/r/8806/#comment32993>

    Would you mind putting the import statements in original order?

src/java/org/apache/sqoop/mapreduce/ExportCallOutputFormat.java
<https://reviews.apache.org/r/8806/#comment33004>

    This method seems to be unused. Would you mind remove it if it's really the case?

src/java/org/apache/sqoop/orm/ClassWriter.java
<https://reviews.apache.org/r/8806/#comment32994>

    Would you mind putting the import statements in original order?

src/test/com/cloudera/sqoop/TestConnFactory.java
<https://reviews.apache.org/r/8806/#comment32995>

    Would you mind putting the import statements in original order?

src/test/com/cloudera/sqoop/TestExport.java
<https://reviews.apache.org/r/8806/#comment32996>

    Would you mind putting the import statements in original order?

src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java
<https://reviews.apache.org/r/8806/#comment32997>

    Would you mind putting the import statements in original order?

src/test/com/cloudera/sqoop/testutil/ExportJobTestCase.java
<https://reviews.apache.org/r/8806/#comment32998>

    Would you mind putting the import statements in original order?
Thank you very much for all your hard work, I really do appreciate that!

Jarcec

- Jarek Cecho
On Jan. 15, 2013, 3:20 a.m., Nick White wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8806/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2013, 3:20 a.m.)
>
>
> Review request for Sqoop and Jarek Cecho.
>
>
> Description
> -------
>
> It'd be useful if you could use stored procedures (or functions) to insert data - currently you can only use insert or update statements (or upsert / merges, depending on the SqlManager you're using). This would help sqoop adoption / migration into environments which have existing, SQL-based data import workflows.