You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Jarek Cecho <ja...@apache.org> on 2015/08/19 02:16:26 UTC
Review Request 37596: SQOOP-2441 Sqoop2: Generic JDBC: Drop support
for specifying custom query when exporting data
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37596/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-2441
https://issues.apache.org/jira/browse/SQOOP-2441
Repository: sqoop-sqoop2
Description
-------
I've dropped all code relevant to this feature.
The patch doesn't deal with upgrade path in case that someone is using this feature to a world where this feature is not available. As the feature is completely broken (e.g. doesn't work at all), I'm kind of assuming that it shouldn't be a problem.
Diffs
-----
common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java 9a9bb66
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java f97e731
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfig.java c9651d5
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestGenericJdbcConnectorUpgrader.java c39aabc
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 870ce98
Diff: https://reviews.apache.org/r/37596/diff/
Testing
-------
Unit tests are passing.
Thanks,
Jarek Cecho
Re: Review Request 37596: SQOOP-2441 Sqoop2: Generic JDBC: Drop
support for specifying custom query when exporting data
Posted by Jarek Cecho <ja...@apache.org>.
> On Aug. 21, 2015, 9:16 p.m., Abraham Elmahrek wrote:
> > It seems good to me except you're missing some of the upgrade logic. Or perhaps removed fields simply won't be transfered? Either way, a warning printed when upgrading might be a good idea. Or at least something in the logs.
Good point, I should have been more descriptive in my comment. The upgrade path is tricky. Our existing logic is "you need to enter either 'table' or 'sql'" and this patch drops the 'sql'. Hence during upgrade, if user had configured 'sql', the job object became invalid and thus the entire upgrade fails. Forcing user to firstly reconfigure their jobs and drop the 'sql' field value before the ugprade.
I'm not anticipating this to be a huge problem because as I've mentioned on the JIRA, this functionality is currently completely broken and doesn't work. Hence I'm not expecting users to have objects with 'sql' filled up.
I completely agree that this is far from ideal state, but I don't see a better option how to remove required fields with current infrastructure. I was thinking about introducing upgrade mode that would disable objects during failed upgrade, but that seems outside of scope for this JIRA. I'm open to feedback though if you have any better idea!
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37596/#review96093
-----------------------------------------------------------
On Aug. 20, 2015, 4:21 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37596/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2015, 4:21 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2441
> https://issues.apache.org/jira/browse/SQOOP-2441
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> I've dropped all code relevant to this feature.
>
> The patch doesn't deal with upgrade path in case that someone is using this feature to a world where this feature is not available. As the feature is completely broken (e.g. doesn't work at all), I'm kind of assuming that it shouldn't be a problem.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java 9a9bb66
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java ad375fd
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfig.java c9651d5
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestGenericJdbcConnectorUpgrader.java c39aabc
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 870ce98
>
> Diff: https://reviews.apache.org/r/37596/diff/
>
>
> Testing
> -------
>
> Unit tests are passing.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 37596: SQOOP-2441 Sqoop2: Generic JDBC: Drop
support for specifying custom query when exporting data
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37596/#review96093
-----------------------------------------------------------
Ship it!
It seems good to me except you're missing some of the upgrade logic. Or perhaps removed fields simply won't be transfered? Either way, a warning printed when upgrading might be a good idea. Or at least something in the logs.
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestGenericJdbcConnectorUpgrader.java (line 35)
<https://reviews.apache.org/r/37596/#comment151307>
Maybe add a test here to show that the removal works.
- Abraham Elmahrek
On Aug. 20, 2015, 4:21 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37596/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2015, 4:21 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2441
> https://issues.apache.org/jira/browse/SQOOP-2441
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> I've dropped all code relevant to this feature.
>
> The patch doesn't deal with upgrade path in case that someone is using this feature to a world where this feature is not available. As the feature is completely broken (e.g. doesn't work at all), I'm kind of assuming that it shouldn't be a problem.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java 9a9bb66
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java ad375fd
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfig.java c9651d5
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestGenericJdbcConnectorUpgrader.java c39aabc
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 870ce98
>
> Diff: https://reviews.apache.org/r/37596/diff/
>
>
> Testing
> -------
>
> Unit tests are passing.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 37596: SQOOP-2441 Sqoop2: Generic JDBC: Drop
support for specifying custom query when exporting data
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37596/#review96206
-----------------------------------------------------------
Ship it!
Ship It!
- Abraham Elmahrek
On Aug. 20, 2015, 4:21 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37596/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2015, 4:21 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2441
> https://issues.apache.org/jira/browse/SQOOP-2441
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> I've dropped all code relevant to this feature.
>
> The patch doesn't deal with upgrade path in case that someone is using this feature to a world where this feature is not available. As the feature is completely broken (e.g. doesn't work at all), I'm kind of assuming that it shouldn't be a problem.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java 9a9bb66
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java ad375fd
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfig.java c9651d5
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestGenericJdbcConnectorUpgrader.java c39aabc
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 870ce98
>
> Diff: https://reviews.apache.org/r/37596/diff/
>
>
> Testing
> -------
>
> Unit tests are passing.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 37596: SQOOP-2441 Sqoop2: Generic JDBC: Drop
support for specifying custom query when exporting data
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37596/
-----------------------------------------------------------
(Updated Aug. 20, 2015, 4:21 p.m.)
Review request for Sqoop.
Changes
-------
Rebased patch on current head.
Bugs: SQOOP-2441
https://issues.apache.org/jira/browse/SQOOP-2441
Repository: sqoop-sqoop2
Description
-------
I've dropped all code relevant to this feature.
The patch doesn't deal with upgrade path in case that someone is using this feature to a world where this feature is not available. As the feature is completely broken (e.g. doesn't work at all), I'm kind of assuming that it shouldn't be a problem.
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java 9a9bb66
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java ad375fd
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfig.java c9651d5
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestGenericJdbcConnectorUpgrader.java c39aabc
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 870ce98
Diff: https://reviews.apache.org/r/37596/diff/
Testing
-------
Unit tests are passing.
Thanks,
Jarek Cecho