----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37121/#review94643 -----------------------------------------------------------
Thanks for the patch Colin! I've just preliminary skimmed through the patch and I have few comments: 1) Please clean up the trailing whitespaces. 2) I would suggest to use SQL_MODE to solve the problem with escaping object names with double quotes [1] Links: 1: https://dev.mysql.com/doc/refman/5.0/en/sql-mode.html common-test/src/main/java/org/apache/sqoop/common/test/db/MySQLProvider.java (lines 96 - 107) <https://reviews.apache.org/r/37121/#comment149230> Please create a first class method in DatabaseProvider named "dropDatabase" rather then re-using dropSchema to dropDatabase and implementing this only for MySQL. common/src/main/java/org/apache/sqoop/error/code/MySqlRepoError.java (line 31) <https://reviews.apache.org/r/37121/#comment149231> Nit: Please end this line with comma "," and pu the semicolon on it's own line. This way future addition of error codes is simpler and doesn't require to change to any of the existing lines. repository/repository-mysql/pom.xml (lines 48 - 51) <https://reviews.apache.org/r/37121/#comment149232> I think that we need to mark this as a provided dependency right? Otherwise we will package the JDBC driver which we can't do due to incompatible licenses. repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MySqlRepositoryHandler.java (lines 157 - 177) <https://reviews.apache.org/r/37121/#comment149233> Can't we just do ON DUPLICATE KEY UPDATE? https://dev.mysql.com/doc/refman/5.0/en/insert-on-duplicate.html repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MysqlRepositoryInsertUpdateDeleteSelectQuery.java (lines 20 - 79) <https://reviews.apache.org/r/37121/#comment149234> Let's do * import here. repository/repository-mysql/src/test/resource/testng.xml (lines 19 - 62) <https://reviews.apache.org/r/37121/#comment149235> I don't think that we need to be so heavy with our own suite.xml file right? We have it only for the integration tests in test module to avoid the cost of starting all the providiers on every class. I don't see the same benefits here, so let's perhaps use the default suite runner? Jarcec - Jarek Cecho On Aug. 5, 2015, 7:38 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37121/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2015, 7:38 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > The sqoop-repository-mysql should be implemented with the > sqoop-repository-comm. > > > Diffs > ----- > > > common-test/src/main/java/org/apache/sqoop/common/test/db/MySQLProvider.java > 3083ee6 > common/src/main/java/org/apache/sqoop/error/code/MySqlRepoError.java > PRE-CREATION > pom.xml d52c4f6 > repository/pom.xml c63595c > repository/repository-mysql/pom.xml PRE-CREATION > > repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MySqlRepoConstants.java > PRE-CREATION > > repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MySqlRepositoryHandler.java > PRE-CREATION > > repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MySqlSchemaCreateQuery.java > PRE-CREATION > > repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MySqlSchemaQuery.java > PRE-CREATION > > repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MysqlRepositoryInsertUpdateDeleteSelectQuery.java > PRE-CREATION > > repository/repository-mysql/src/test/java/org/apache/sqoop/integration/repository/mysql/MySqlTestCase.java > PRE-CREATION > > repository/repository-mysql/src/test/java/org/apache/sqoop/integration/repository/mysql/MySqlTestUtils.java > PRE-CREATION > > repository/repository-mysql/src/test/java/org/apache/sqoop/integration/repository/mysql/TestConnectorHandling.java > PRE-CREATION > > repository/repository-mysql/src/test/java/org/apache/sqoop/integration/repository/mysql/TestDriverHandling.java > PRE-CREATION > > repository/repository-mysql/src/test/java/org/apache/sqoop/integration/repository/mysql/TestHandler.java > PRE-CREATION > > repository/repository-mysql/src/test/java/org/apache/sqoop/integration/repository/mysql/TestJobHandling.java > PRE-CREATION > > repository/repository-mysql/src/test/java/org/apache/sqoop/integration/repository/mysql/TestLinkHandling.java > PRE-CREATION > > repository/repository-mysql/src/test/java/org/apache/sqoop/integration/repository/mysql/TestStructure.java > PRE-CREATION > > repository/repository-mysql/src/test/java/org/apache/sqoop/integration/repository/mysql/TestSubmissionHandling.java > PRE-CREATION > repository/repository-mysql/src/test/resource/testng.xml PRE-CREATION > server/pom.xml aabefc0 > > Diff: https://reviews.apache.org/r/37121/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
