> On Sept. 14, 2012, 1:26 a.m., Cheolsoo Park wrote: > > Thank you very much for adding many tests for the PG connector. > > > > Looks good overall. I have a few minor comments below.
Hi Chelsoo, thank you very much for your review. I appreciate your input, please see my comments below: > On Sept. 14, 2012, 1:26 a.m., Cheolsoo Park wrote: > > src/java/org/apache/sqoop/manager/PostgresqlManager.java, line 69 > > <https://reviews.apache.org/r/7055/diff/1/?file=152913#file152913line69> > > > > Is 'ignored' needed at all? If not, can you delete this constructor all > > together? I've originally didn't want to change this as I didn't introduced this constructor, but I've removed it. > On Sept. 14, 2012, 1:26 a.m., Cheolsoo Park wrote: > > src/java/org/apache/sqoop/manager/PostgresqlManager.java, line 205 > > <https://reviews.apache.org/r/7055/diff/1/?file=152913#file152913line205> > > > > I suppose that you have a reason why you use GnuParser instead of > > SqoopParser. If so, can you please make the comment mode informative? I've added comment that I do not need extended abilities provided by SqoopParser. > On Sept. 14, 2012, 1:26 a.m., Cheolsoo Park wrote: > > src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java, line 44 > > <https://reviews.apache.org/r/7055/diff/1/?file=152920#file152920line44> > > > > I believe that this is a typo: Postgress*Export*Test. Nope, it's not a typo. Project name is PostgreSQL: http://www.postgresql.org/ > On Sept. 14, 2012, 1:26 a.m., Cheolsoo Park wrote: > > src/test/com/cloudera/sqoop/manager/PostgresqlExportTest.java, line 112 > > <https://reviews.apache.org/r/7055/diff/1/?file=152920#file152920line112> > > > > 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? I've used internal test escape method instead. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7055/#review11512 ----------------------------------------------------------- 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 > >
