----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41180/#review109736 -----------------------------------------------------------
Overall looks good to me. I have couple of comments inline. It seems that there is findbugs warning and test coverage decrease significantly - perhaps we should improve unit test coverage for the new code? connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java (lines 34 - 35) <https://reviews.apache.org/r/41180/#comment169363> Good example of tests that we should add is the logic in readFields/writeWields - since we have now quite a lot of logic there what about writing a test cases for the serializatin/deserialization? connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java (line 56) <https://reviews.apache.org/r/41180/#comment169362> Shouldn't we add a space or something here when creating multi condition partition? connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartitioner.java (lines 20 - 37) <https://reviews.apache.org/r/41180/#comment169364> Seems as not relevant change? (just moving lines around that doesn't do anything) connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestPartitioner.java (lines 20 - 35) <https://reviews.apache.org/r/41180/#comment169365> Again seem as not relevant import-reordering? Jarcec - Jarek Cecho On Dec. 10, 2015, 2:28 a.m., Abraham Fine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41180/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2015, 2:28 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2727 > https://issues.apache.org/jira/browse/SQOOP-2727 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Sqoop2: Use PreparedStatements for constructing GenericJDBCPartitions > > > Diffs > ----- > > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java > ff33a4b > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExtractor.java > edb2754 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java > fa26c14 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java > 65400ef > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartitioner.java > 2a42ed4 > > connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java > 3b52128 > > connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestPartitioner.java > 3a767ab > > Diff: https://reviews.apache.org/r/41180/diff/ > > > Testing > ------- > > yup > > > Thanks, > > Abraham Fine > >
