----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38881/#review101123 -----------------------------------------------------------
Hi David, thank you for porting the code! I wasn't able to go through entire code yet, but I figured that it's better to share the notes that I have so far :) 1) We're keeping the test/ module for integration tests with the whole server running as an external component and on minicluster. Component specific integration tests or for tests with external dependencies, should be in it's component specific package. We're using special groups to mark those tests as not running by default. For example for postgresql: https://github.com/apache/sqoop/blob/sqoop2/repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestConnectorHandling.java#L33 And then handling in the appropriate pom.xml file: https://github.com/apache/sqoop/blob/sqoop2/repository/repository-postgresql/pom.xml#L75 2) I'm more then happy to commit not fully finished (but working!) version of the connector and then tune it in subsequent JIRAs. We've chose this approach for most of the connectors and since Sqoop 2 is not mature yet, it's working fine for us at this point. connector/connector-oracle-jdbc/pom.xml (lines 41 - 44) <https://reviews.apache.org/r/38881/#comment158456> Generally we don't like inter-dependencies on connectors. If there is a reason for this dependency (reusing some code), then we should refactore the shared code to connector-sdk module. connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/configuration/FromJobConfig.java (lines 62 - 69) <https://reviews.apache.org/r/38881/#comment158460> You'll be better of with using standard validator NotEmpty: https://github.com/apache/sqoop/blob/sqoop2/common/src/main/java/org/apache/sqoop/validation/validators/NotEmpty.java Soon-ish the standard validators will be enforced on client side as well which will provide a slightly better user experience :) connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/configuration/LinkConfig.java (line 36) <https://reviews.apache.org/r/38881/#comment158467> I know that the reference for this class is the Generic JDBC Connector, but I reall hate the naming convenction we've used there and now it's hard to rename. Can we call this class ConnectionConfig or something? Since most of the inputs are for connection. The Confings should contain only inputs that belongs together logically - if we have an input that is not describing connection, we should move it to different Config. Looking at it it seems that most of the inputs are for connection (and or session), the only question that I have I guess is whether actionName and actionName is indeed relevant to "connection". But that is up to you :) The same arguments (let's use better name and perhaps split the single Configs to multiple ones) applies to other Config classes for this connector. connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/configuration/LinkConfig.java (line 59) <https://reviews.apache.org/r/38881/#comment158472> Since we're expecting multiple statements here, would it make sense to convert this type to List<String> instead? Jarcec - Jarek Cecho On Sept. 30, 2015, 4:30 a.m., David Robson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38881/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2015, 4:30 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2595 > https://issues.apache.org/jira/browse/SQOOP-2595 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Have migrated the code from the Oracle direct connector to Sqoop 2. There is > still work to be done but it is working so I would like to get some feedback > on the work so far. As the code is limited to one connector I am hoping we > can commit it then fix issues in follow up JIRAs as it will make it a bit > easier to break the tasks down. > > > Diffs > ----- > > connector/connector-oracle-jdbc/pom.xml PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcCommonInitializer.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcConnector.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcConnectorConstants.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcConnectorUpgrader.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcExtractor.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcFromDestroyer.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcFromInitializer.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcLoader.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcPartition.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcPartitioner.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcToDestroyer.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/OracleJdbcToInitializer.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/configuration/FromJobConfig.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/configuration/FromJobConfiguration.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/configuration/LinkConfig.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/configuration/LinkConfiguration.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/configuration/ToJobConfig.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/configuration/ToJobConfiguration.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleActiveInstance.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleConnectionFactory.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleDataChunk.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleDataChunkExtent.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleDataChunkPartition.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleGenerics.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleJdbcUrl.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleQueries.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleSqlTypesUtils.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleTable.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleTableColumn.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleTableColumns.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleTablePartition.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleTablePartitions.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleUtilities.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/oracle/util/OracleVersion.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/resources/oracle-jdbc-connector-config.properties > PRE-CREATION > > connector/connector-oracle-jdbc/src/main/resources/sqoopconnector.properties > PRE-CREATION > > connector/connector-oracle-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/oracle/TestOracleJdbcPartitioner.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/oracle/TestOracleJdbcUrl.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/oracle/TestOracleTable.java > PRE-CREATION > > connector/connector-oracle-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/oracle/TestOracleUtilities.java > PRE-CREATION > connector/pom.xml 1b69180 > pom.xml ef3f5f4 > server/pom.xml 59663fa > test/pom.xml 8218477 > > test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java > a0ef78a > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/ExportTest.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/ImportTest.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/OracleConnectionFactoryTest.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/OracleQueriesTest.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/OracleTestCase.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/OracleTestConstants.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/BigDecimalGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/BinaryDoubleGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/BinaryFloatGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/BlobGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/BytesGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/CharGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/FloatGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/HadoopFiles.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/IntervalDaySecondGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/IntervalYearMonthGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/NCharGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/OracleData.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/OracleDataDefinition.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/OracleTableDefinition.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/OracleTestDataGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/OracleTestUtils.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/RowIdGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/TimestampGenerator.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/oracle/util/URIGenerator.java > PRE-CREATION > test/src/test/resources/oracle/create_users.sql PRE-CREATION > test/src/test/resources/oracle/pkg_tst_product_gen.pbk PRE-CREATION > test/src/test/resources/oracle/pkg_tst_product_gen.psk PRE-CREATION > test/src/test/resources/oracle/table_tst_product.xml PRE-CREATION > test/src/test/resources/oracle/table_tst_product_part.xml PRE-CREATION > test/src/test/resources/oracle/table_tst_product_part_iot.xml PRE-CREATION > test/src/test/resources/oracle/table_tst_product_special_chars.xml > PRE-CREATION > test/src/test/resources/oracle/table_tst_product_subpart.xml PRE-CREATION > > Diff: https://reviews.apache.org/r/38881/diff/ > > > Testing > ------- > > Have done lots of manual testing and migrated some of the automated tests. > Some of them I am having issues with which I will follow up. > > > Thanks, > > David Robson > >
