----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29749/#review70624 -----------------------------------------------------------
I'm wondering what is the reason behind removing the ProviderAssert class? common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java <https://reviews.apache.org/r/29749/#comment115877> Since we do have a boolean method isSupportingScheme() that is suppose to return true/false whether schemes are support, would it make sense to make this method concrete and let the default implementation throw NotImplementedException? I know that this is a test class, so API purity is probably not the first goal :) common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java <https://reviews.apache.org/r/29749/#comment115881> Do you think that we can simplify that a bit? Perhaps: sb.append(escape ? escapeTableName(tableName) : tableName) common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java <https://reviews.apache.org/r/29749/#comment115880> Shouldn't we use the newly introduced method "escapeSchemaName()" here? common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java <https://reviews.apache.org/r/29749/#comment115882> Shouldn't the default be: stmt.setObject() ? common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java <https://reviews.apache.org/r/29749/#comment115883> I think that we should keep the method rowCount(tableName) around as well in addition to the rowCount(schemaName, tableName). common-test/src/main/java/org/apache/sqoop/common/test/db/MySQLProvider.java <https://reviews.apache.org/r/29749/#comment115884> MySQL doesn't have concept of schema, so this do seem a bit weird. repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java <https://reviews.apache.org/r/29749/#comment115885> Nit: I would probably kept the original version with static imports of all the various contants, seems as better readable code, but I don't feel that strongly. repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/PostgresqlTestUtils.java <https://reviews.apache.org/r/29749/#comment115912> Don't we have generic ProviderAsserts? Those method seems to be generic enough to be reused on other databases as well, right? - Jarek Cecho On Jan. 31, 2015, 12:11 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29749/ > ----------------------------------------------------------- > > (Updated Jan. 31, 2015, 12:11 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1591 > https://issues.apache.org/jira/browse/SQOOP-1591 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit 560387f1d3e6869ada914fa6ae5ada7d8e210721 > Author: Abraham Elmahrek <[email protected]> > Date: Wed Jan 7 20:44:44 2015 -0800 > > SQOOP-1591: Sqoop2: PostgreSQL integration tests > > :100644 100644 8196fe2... cb987f6... M > common-test/src/main/java/org/apache/sqoop/common/test/asserts/ProviderAsserts.java > :100644 100644 82289e8... 3a6205e... M > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java > :100644 100644 5d80dce... 7248d7c... M > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java > :000000 100644 0000000... 85f895d... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestConnectorHandling.java > :000000 100644 0000000... 6022bb3... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestDriverHandling.java > :000000 100644 0000000... 1dfacb5... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestJobHandling.java > :000000 100644 0000000... 54c598e... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestLinkHandling.java > :100644 100644 2da19bc... 4735240... M > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestStructure.java > :000000 100644 0000000... 9db7940... A > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestSubmissionHandling.java > > > Diffs > ----- > > > common-test/src/main/java/org/apache/sqoop/common/test/asserts/ProviderAsserts.java > fb4e7af > > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java > 82289e8 > > common-test/src/main/java/org/apache/sqoop/common/test/db/DerbyProvider.java > 98591a3 > > common-test/src/main/java/org/apache/sqoop/common/test/db/MySQLProvider.java > 9814ac8 > > common-test/src/main/java/org/apache/sqoop/common/test/db/NetezzaProvider.java > d31bf28 > > common-test/src/main/java/org/apache/sqoop/common/test/db/OracleProvider.java > ed29a23 > > common-test/src/main/java/org/apache/sqoop/common/test/db/PostgreSQLProvider.java > d46e01d > > common-test/src/main/java/org/apache/sqoop/common/test/db/SqlServerProvider.java > 9c56886 > > common-test/src/main/java/org/apache/sqoop/common/test/db/TeradataProvider.java > f99d1ed > repository/repository-postgresql/pom.xml 0ee9081 > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoConstants.java > bdefd4c > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java > 85af9a4 > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/PostgresqlTestCase.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/PostgresqlTestUtils.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestConnectorHandling.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestDriverHandling.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestJobHandling.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestLinkHandling.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestStructure.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/TestSubmissionHandling.java > PRE-CREATION > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java > 98ba7a3 > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestUtils.java > 19fd6e7 > > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestStructure.java > 941bb69 > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > 1124cd3 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromHDFSToRDBMSTest.java > f82abc7 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedRDBMSTest.java > b648870 > > Diff: https://reviews.apache.org/r/29749/diff/ > > > Testing > ------- > > mvn clean test -pl repository/repository-postgresql > -Dsqoop.provider.class=org.apache.sqoop.common.test.db.PostgreSQLProvider > -Dsqoop.provider.postgresql.jdbc=jdbc:postgresql://.../test > -Dsqoop.provider.postgresql.username=test > -Dsqoop.provider.postgresql.password=test > > > Thanks, > > Abraham Elmahrek > >
