You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Gwen Shapira <gs...@cloudera.com> on 2014/05/29 21:53:20 UTC
Review Request 22034: SQOOP 1331 - Add method "findConnectors" to repository
API
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22034/
-----------------------------------------------------------
Review request for Sqoop.
Repository: sqoop-sqoop2
Description
-------
Adding a method to get all connectors to the repository API.
Note that while this patch adds a new method, it also modifies the existing findConnector(String shortName) to avoid duplicating code.
Diffs
-----
core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 7768b13
core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java 9299484
core/src/main/java/org/apache/sqoop/repository/Repository.java 4c7b866
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java e4b30f9
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 4f002bb
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java c470211
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java a1ad40d
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java cc3fe60
Diff: https://reviews.apache.org/r/22034/diff/
Testing
-------
Added unit-test for the new method. Did basic repository functionality testing on a cluster.
Thanks,
Gwen Shapira
Re: Review Request 22034: SQOOP 1331 - Add method "findConnectors" to
repository API
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22034/#review44322
-----------------------------------------------------------
Couple of other comments:
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/22034/#comment78697>
I would put the debug message as the first statement in the method rather then at the end.
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/22034/#comment78696>
Removing the finally section do not seem to be completely accurate here, we still want to close the baseConnectorFetchStmt, right?
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java
<https://reviews.apache.org/r/22034/#comment78698>
Nit: Please use the one line comment that starts with "//"
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java
<https://reviews.apache.org/r/22034/#comment78699>
Super nit: please put space between "//" and "Retrieve"
- Jarek Cecho
On May 29, 2014, 7:53 p.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22034/
> -----------------------------------------------------------
>
> (Updated May 29, 2014, 7:53 p.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Adding a method to get all connectors to the repository API.
>
> Note that while this patch adds a new method, it also modifies the existing findConnector(String shortName) to avoid duplicating code.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 7768b13
> core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java 9299484
> core/src/main/java/org/apache/sqoop/repository/Repository.java 4c7b866
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java e4b30f9
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 4f002bb
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java c470211
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java a1ad40d
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java cc3fe60
>
> Diff: https://reviews.apache.org/r/22034/diff/
>
>
> Testing
> -------
>
> Added unit-test for the new method. Did basic repository functionality testing on a cluster.
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 22034: SQOOP 1331 - Add method "findConnectors" to
repository API
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22034/#review44470
-----------------------------------------------------------
Ship it!
Please attach the patch to the JIRA and I'll commit it.
- Jarek Cecho
On May 30, 2014, 4:30 p.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22034/
> -----------------------------------------------------------
>
> (Updated May 30, 2014, 4:30 p.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Adding a method to get all connectors to the repository API.
>
> Note that while this patch adds a new method, it also modifies the existing findConnector(String shortName) to avoid duplicating code.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 7768b13
> core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java 9299484
> core/src/main/java/org/apache/sqoop/repository/Repository.java 4c7b866
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java e4b30f9
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 4f002bb
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java c470211
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java a1ad40d
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java cc3fe60
>
> Diff: https://reviews.apache.org/r/22034/diff/
>
>
> Testing
> -------
>
> Added unit-test for the new method. Did basic repository functionality testing on a cluster.
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 22034: SQOOP 1331 - Add method "findConnectors" to
repository API
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22034/
-----------------------------------------------------------
(Updated May 30, 2014, 4:30 p.m.)
Review request for Sqoop.
Changes
-------
Fixed the issues mentioned below:
* added "finally" clause to close the base statement.
* streamlined the conditions
* straightened out the comments
Repository: sqoop-sqoop2
Description
-------
Adding a method to get all connectors to the repository API.
Note that while this patch adds a new method, it also modifies the existing findConnector(String shortName) to avoid duplicating code.
Diffs (updated)
-----
core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 7768b13
core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java 9299484
core/src/main/java/org/apache/sqoop/repository/Repository.java 4c7b866
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java e4b30f9
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 4f002bb
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java c470211
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java a1ad40d
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java cc3fe60
Diff: https://reviews.apache.org/r/22034/diff/
Testing
-------
Added unit-test for the new method. Did basic repository functionality testing on a cluster.
Thanks,
Gwen Shapira
Re: Review Request 22034: SQOOP 1331 - Add method "findConnectors" to
repository API
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22034/#review44319
-----------------------------------------------------------
Ship it!
Couple of nits but looks good.
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/22034/#comment78693>
} else if (...?
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/22034/#comment78692>
Spacing seems a bit off. Maybe just reviewboard?
- Abraham Elmahrek
On May 29, 2014, 7:53 p.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22034/
> -----------------------------------------------------------
>
> (Updated May 29, 2014, 7:53 p.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Adding a method to get all connectors to the repository API.
>
> Note that while this patch adds a new method, it also modifies the existing findConnector(String shortName) to avoid duplicating code.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 7768b13
> core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java 9299484
> core/src/main/java/org/apache/sqoop/repository/Repository.java 4c7b866
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java e4b30f9
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 4f002bb
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java c470211
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java a1ad40d
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java cc3fe60
>
> Diff: https://reviews.apache.org/r/22034/diff/
>
>
> Testing
> -------
>
> Added unit-test for the new method. Did basic repository functionality testing on a cluster.
>
>
> Thanks,
>
> Gwen Shapira
>
>