----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7055/#review11512 -----------------------------------------------------------
Thank you very much for adding many tests for the PG connector. Looks good overall. I have a few minor comments below. src/java/org/apache/sqoop/manager/PostgresqlManager.java <https://reviews.apache.org/r/7055/#comment24723> Is 'ignored' needed at all? If not, can you delete this constructor all together? src/java/org/apache/sqoop/manager/PostgresqlManager.java <https://reviews.apache.org/r/7055/#comment24730> I suppose that you have a reason why you use GnuParser instead of SqoopParser. If so, can you please make the comment mode informative? src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java <https://reviews.apache.org/r/7055/#comment24724> I believe that this is a typo: Postgress*Export*Test. src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java <https://reviews.apache.org/r/7055/#comment24733> This comment applies to line #112 and #120. Using escapeTableName() for both schema and tableName is a bit confusing to me. Wouldn't it be better to do the following? 1) Make getSchemaSqlFragment() public in PostgresqlManager. 2) Add a setter such as setSchema() to PostgresqlManager. Now you can do: manager.setSchema(schema); ... st.executeUpdate("CREATE SCHEMA " + manager.getSchemaSqlFragment()); ... String fullTableName = manager.escapeTableName(tableName); ... I find this cleaner. What do you think? - Cheolsoo Park On Sept. 12, 2012, 12:09 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7055/ > ----------------------------------------------------------- > > (Updated Sept. 12, 2012, 12:09 p.m.) > > > Review request for Sqoop. > > > Description > ------- > > This patch is adding support for custom schemas into PostgreSQL manager. > Changes for import job were very simple and rather straightforward. On the > other side, changes for export job were little tricky, but I believe that > I've succeeded without breaking backward compatibility. > > > Diffs > ----- > > src/docs/user/connectors.txt a93f14e > src/java/org/apache/sqoop/manager/CatalogQueryManager.java 5f2f89f > src/java/org/apache/sqoop/manager/PostgresqlManager.java d18321c > src/java/org/apache/sqoop/manager/SqlManager.java ea961cd > src/java/org/apache/sqoop/mapreduce/JdbcExportJob.java b574f82 > src/java/org/apache/sqoop/mapreduce/JdbcUpdateExportJob.java 7be5ed9 > src/java/org/apache/sqoop/mapreduce/JdbcUpsertExportJob.java f299f98 > src/java/org/apache/sqoop/mapreduce/db/DataDrivenDBRecordReader.java > 38e9fb9 > src/test/com/cloudera/sqoop/ThirdPartyTests.java eeab7f3 > src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java PRE-CREATION > src/test/com/cloudera/sqoop/manager/PostgresqlImportTest.java PRE-CREATION > src/test/com/cloudera/sqoop/manager/PostgresqlTest.java 0dfd1fc > > Diff: https://reviews.apache.org/r/7055/diff/ > > > Testing > ------- > > I've added a lot of third party tests (e.g. they won't be run automatically > during "ant test") to properly test various combinations of parameters. You > can invoke those tests issuing one of following commands: > > ant test -Dtestcase=PostgresqlImportTest > ant test -Dtestcase=PostgresqlExportTest > > I've tested compatibility by running ant test -Dhadoopversion={20,100,23,200} > > > Thanks, > > Jarek Cecho > >
