----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40896/#review109853 -----------------------------------------------------------
Thanks for the patch Abe, appreciated! Few notes: common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java (lines 273 - 296) <https://reviews.apache.org/r/40896/#comment169509> It seems that the other methods are re-trowing the SQLExcepstion as runtime - can we perhaps stick to this idiom then have one method that behaves differently? common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java (lines 335 - 368) <https://reviews.apache.org/r/40896/#comment169510> Can we abstract this behavior and use it in insertRow as well? connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java (lines 34 - 35) <https://reviews.apache.org/r/40896/#comment169512> Can we add unit tests for the newly added functionality? The class was before "dumb" so we didn't have any, but now it has non-trivial logic inside so we should test it. connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java (lines 55 - 57) <https://reviews.apache.org/r/40896/#comment169515> In case that we don't need appending, can we change this method to "setCondition"? connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartitioner.java (lines 20 - 37) <https://reviews.apache.org/r/40896/#comment169514> Nit: Please remove the import reordering. connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartitioner.java (line 517) <https://reviews.apache.org/r/40896/#comment169516> It seems taht the dateType will be of value Types.DATE, Types.TIMESTAMP and Types.TIME, but we are always storing String (output of sdf.format). Should we then store the type as String or use the lowerBround directly? connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestPartitioner.java (lines 20 - 35) <https://reviews.apache.org/r/40896/#comment169517> Nit: Please remove the import re-ordering. connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestPartitioner.java (line 252) <https://reviews.apache.org/r/40896/#comment169519> Would you mind describing what exactly is going wrong with the original implementation? I'm failing to see the bug sadly. - Jarek Cecho On Dec. 10, 2015, 9:03 p.m., Abraham Fine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40896/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2015, 9:03 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2719 > https://issues.apache.org/jira/browse/SQOOP-2719 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Sqoop2: Ensure the GenericJDBCConnector integration tests work against MySQL, > PostgreSQL, and Oracle > > > Diffs > ----- > > > common-test/src/main/java/org/apache/sqoop/common/test/asserts/ProviderAsserts.java > 0da7ea8 > > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java > f30d587 > > common-test/src/main/java/org/apache/sqoop/common/test/db/MySQLProvider.java > 393904f > > 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 > test/src/main/java/org/apache/sqoop/test/data/Cities.java fbbd7ef > test/src/main/java/org/apache/sqoop/test/data/DataSet.java 5e109b1 > test/src/main/java/org/apache/sqoop/test/data/ShortStories.java 66bf2cd > test/src/main/java/org/apache/sqoop/test/data/UbuntuReleases.java 5b75389 > test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java > c1f355f > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > c843448 > > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/AppendModeTest.java > 8c65898 > > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/FromHDFSToHDFSTest.java > c39c8d6 > > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/HdfsIncrementalReadTest.java > e6f6e0d > > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/InformalJobNameExecuteTest.java > 411b07e > > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/OutputDirectoryTest.java > 1790f96 > > test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java > e964422 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java > 5053b56 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromHDFSToRDBMSTest.java > 25cdb68 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java > 686572a > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java > f850768 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/PartitionerTest.java > 42bff65 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedRDBMSTest.java > 68dc65e > > test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java > 4ecb018 > > test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java > 7b2aced > > test/src/test/java/org/apache/sqoop/integration/server/SubmissionWithDisabledModelObjectsTest.java > b309c86 > > Diff: https://reviews.apache.org/r/40896/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Abraham Fine > >
