----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37947/#review97276 -----------------------------------------------------------
Thank you for the change Dian! Overall it looks good I have few nits: 1) Can we add integration test validating that the upgrade indeed works? We can perhaps add repository dump for 1.99.5 and/or 1.99.6 that will have the empty names and try upgrading? Something like [1]. 2) Can we also alter the shell to request user to specify the name as part of providing inputs? Links: 1: https://github.com/apache/sqoop/blob/sqoop2/test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_4UpgradeTest.java common/src/main/java/org/apache/sqoop/utils/SqoopUtils.java (line 18) <https://reviews.apache.org/r/37947/#comment153109> It seems that this class is used only for integration tests - let's perhaps move to the integration tests package? tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java (line 297) <https://reviews.apache.org/r/37947/#comment153110> I would recommend not filling names for the repository load/dump tool - if the backup can't be loaded we should fail rather then altering it. Jarcec - Jarek Cecho On Sept. 1, 2015, 12:51 a.m., Dian Fu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37947/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2015, 12:51 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2539 > https://issues.apache.org/jira/browse/SQOOP-2539 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Currently resource name such as job name, connector name or link name can be > null. We should enforce that these resource names cannot be null. > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/utils/SqoopUtils.java PRE-CREATION > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepoUtils.java > df41fb1 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > 0b68058 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaUpgradeQuery.java > 6adf959 > > repository/repository-mysql/src/main/java/org/apache/sqoop/repository/mysql/MySqlSchemaCreateQuery.java > 46493a3 > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java > 0d0ea8c > > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaUpgradeQuery.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java > 82043ea > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > e3e7bfe > tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java > 244683d > > Diff: https://reviews.apache.org/r/37947/diff/ > > > Testing > ------- > > mvn test > > > Thanks, > > Dian Fu > >
