You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Dian Fu <di...@gmail.com> on 2015/09/01 02:51:02 UTC
Re: Review Request 37947: Sqoop2: Enforce resource name not null when
creating table
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 37947: Sqoop2: Enforce resource name not null when
creating table
Posted by Dian Fu <di...@gmail.com>.
> On Sept. 1, 2015, 11:53 a.m., Jarek Cecho wrote:
> > 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
Thanks Jarcec for your review. The comments make sense to me. Have updated the patch accordingly.
- Dian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37947/#review97276
-----------------------------------------------------------
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
>
>
Re: Review Request 37947: Sqoop2: Enforce resource name not null when
creating table
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 37947: Sqoop2: Enforce resource name not null when
creating table
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37947/#review97987
-----------------------------------------------------------
Ship it!
Small nit below, otherwise I'm +1.
shell/src/main/java/org/apache/sqoop/shell/utils/ConfigFiller.java (line 893)
<https://reviews.apache.org/r/37947/#comment154123>
Nit: Let's replace the check with StringUtils.isEmpty(nameInput.getValue())
https://commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/apache/commons/lang/StringUtils.html#isEmpty(java.lang.String)
Jarcec
- Jarek Cecho
On Sept. 8, 2015, 12:45 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37947/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2015, 12:45 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
> -----
>
> 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
> shell/src/main/java/org/apache/sqoop/shell/utils/ConfigFiller.java 63b1267
> test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java 82043ea
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java e3e7bfe
> test/src/main/java/org/apache/sqoop/test/utils/SqoopUtils.java PRE-CREATION
> test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_5UpgradeTest.java PRE-CREATION
> test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_6UpgradeTest.java PRE-CREATION
> test/src/test/resources/repository/derby/derby-repository-1.99.5.tar.gz PRE-CREATION
> test/src/test/resources/repository/derby/derby-repository-1.99.6.tar.gz PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37947/diff/
>
>
> Testing
> -------
>
> mvn test
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 37947: Sqoop2: Enforce resource name not null when
creating table
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37947/
-----------------------------------------------------------
(Updated Sept. 8, 2015, 10:40 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 (updated)
-----
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 45c7447
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 5ada2d0
repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaUpgradeQuery.java PRE-CREATION
shell/src/main/java/org/apache/sqoop/shell/utils/ConfigFiller.java 63b1267
test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java 82043ea
test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java e3e7bfe
test/src/main/java/org/apache/sqoop/test/utils/SqoopUtils.java PRE-CREATION
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_5UpgradeTest.java PRE-CREATION
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_6UpgradeTest.java PRE-CREATION
test/src/test/resources/repository/derby/derby-repository-1.99.5.tar.gz PRE-CREATION
test/src/test/resources/repository/derby/derby-repository-1.99.6.tar.gz PRE-CREATION
Diff: https://reviews.apache.org/r/37947/diff/
Testing
-------
mvn test
Thanks,
Dian Fu
Re: Review Request 37947: Sqoop2: Enforce resource name not null when
creating table
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37947/
-----------------------------------------------------------
(Updated Sept. 8, 2015, 12:45 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 (updated)
-----
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
shell/src/main/java/org/apache/sqoop/shell/utils/ConfigFiller.java 63b1267
test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java 82043ea
test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java e3e7bfe
test/src/main/java/org/apache/sqoop/test/utils/SqoopUtils.java PRE-CREATION
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_5UpgradeTest.java PRE-CREATION
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_6UpgradeTest.java PRE-CREATION
test/src/test/resources/repository/derby/derby-repository-1.99.5.tar.gz PRE-CREATION
test/src/test/resources/repository/derby/derby-repository-1.99.6.tar.gz PRE-CREATION
Diff: https://reviews.apache.org/r/37947/diff/
Testing
-------
mvn test
Thanks,
Dian Fu