> On Dec. 5, 2015, 2:38 p.m., Jarek Cecho wrote: > > It seems that we have a lot of various fixes inside one patch. I hesitate > > to push this as a single change as we will loose track of why exactly we > > did those changes. Would you mind splitting each individual fix to it's own > > JIRA describing what is the problem and hence why we need to fix it?
my concern with splitting this into a bunch of seperate patches is that many of these changes are interdependent in nonobvious ways (due to database idiosynchracies). I figured that it would be easier to merge it in together so we can have a baseline for working across multiple databases. if you feel strongly about it, i can change it. > On Dec. 5, 2015, 2:38 p.m., Jarek Cecho wrote: > > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java, > > lines 243-247 > > <https://reviews.apache.org/r/40896/diff/5/?file=1154208#file1154208line243> > > > > Out of my curiosity - what database throws RuntimeException when > > dropping non existing table? Shouldn't we catch that inside the dropTable() > > method? (VERIFY) the purpose of dropping tables is to clean up the test environment. in other words, it is not essential that we successfully drop the tables (as they may not actually exist). we get this behavior by default when we run our tests against a mini cluster, but we need to do it manually if we are running against a real cluster and a real database. but after looking at the droptable method, that is what we are doing already. so i can remove my exception catching logic. ```java public void dropTable(TableName tableName) { StringBuilder sb = new StringBuilder("DROP TABLE "); sb.append(getTableFragment(tableName)); try { executeUpdate(sb.toString()); } catch(RuntimeException e) { LOG.info("Ignoring exception: " + e); } } ``` > On Dec. 5, 2015, 2:38 p.m., Jarek Cecho wrote: > > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java, > > lines 366-367 > > <https://reviews.apache.org/r/40896/diff/5/?file=1154208#file1154208line366> > > > > Will this generally work for all databases that you've mentioned? this should work for all three databases. i would not feel comfortable doing this with production code, but for test code i think that it is fine. > On Dec. 5, 2015, 2:38 p.m., Jarek Cecho wrote: > > common-test/src/main/java/org/apache/sqoop/common/test/db/OracleProvider.java, > > lines 67-75 > > <https://reviews.apache.org/r/40896/diff/5/?file=1154210#file1154210line67> > > > > This seems weird to me - if we need to split something with a dot, then > > that something is not a table name. It's a database and table name at the > > same time. I would much rather fix that code to distinguish that difference > > then do this sneaky splitting. you are right. i have no idea why i put this in. - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40896/#review109086 ----------------------------------------------------------- On Dec. 5, 2015, 6:22 a.m., Abraham Fine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40896/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2015, 6:22 a.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/db/DatabaseProvider.java > f30d587 > > common-test/src/main/java/org/apache/sqoop/common/test/db/MySQLProvider.java > 393904f > > common-test/src/main/java/org/apache/sqoop/common/test/db/OracleProvider.java > b5f3104 > > 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 > docs/src/site/sphinx/user/connectors/Connector-GenericJDBC.rst 347547d > 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 b84339e > test/src/main/java/org/apache/sqoop/test/data/UbuntuReleases.java f8aeb38 > test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java > 4c5d3a8 > 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 > 0e46bf3 > > 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 > 38ebb74 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/PartitionerTest.java > 72728fe > > 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 > 6e78a13 > > test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java > 7b2aced > > test/src/test/java/org/apache/sqoop/integration/connectorloading/BlacklistedConnectorTest.java > 6228b0d > > test/src/test/java/org/apache/sqoop/integration/server/ShowJobInOrderTest.java > cbf1e90 > > test/src/test/java/org/apache/sqoop/integration/server/SubmissionWithDisabledModelObjectsTest.java > 9e682bc > > Diff: https://reviews.apache.org/r/40896/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Abraham Fine > >
