----------------------------------------------------------- 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. > > The attached patch adds a --call argument to the export tool that can be used > in place of --table. > > Note that this patch depends on HSQLDB 2.x, whereas trunk currently depends > on 1.8, so it can't be committed until that situation's resolved (see > SQOOP-519 and linked tickets). > > > This addresses bug SQOOP-749. > https://issues.apache.org/jira/browse/SQOOP-749 > > > Diffs > ----- > > ivy.xml 1ee60df > ivy/libraries.properties 4c9e37d > src/docs/user/export-purpose.txt c26eaa7 > src/docs/user/export.txt 9f600fe > src/java/org/apache/sqoop/SqoopOptions.java 3e0ec3e > src/java/org/apache/sqoop/manager/ConnManager.java 21eea93 > src/java/org/apache/sqoop/manager/SqlManager.java 3a52c6d > src/java/org/apache/sqoop/mapreduce/ExportCallOutputFormat.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/JdbcCallExportJob.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/JdbcExportJob.java 7c52110 > src/java/org/apache/sqoop/orm/ClassWriter.java b73711e > src/java/org/apache/sqoop/tool/BaseSqoopTool.java c0221c9 > src/java/org/apache/sqoop/tool/ExportTool.java acd296d > src/test/com/cloudera/sqoop/SmokeTests.java 76df6cf > src/test/com/cloudera/sqoop/TestConnFactory.java 893b388 > src/test/com/cloudera/sqoop/TestExport.java eba10aa > src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java be449e4 > src/test/com/cloudera/sqoop/testutil/ExportJobTestCase.java 4f6fd37 > src/test/org/apache/sqoop/TestExportUsingProcedure.java PRE-CREATION > > Diff: https://reviews.apache.org/r/8806/diff/ > > > Testing > ------- > > I've added a subclass of TestExport to make sure all the usual export > functionality works. I've also used it on live data exporting to postgres. > > > Thanks, > > Nick White > >
