You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Raghav Gautam <ra...@gmail.com> on 2013/09/07 02:18:06 UTC
Review Request 14022: SQOOP-974 Support for stage table
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14022/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: sqoop-974
https://issues.apache.org/jira/browse/sqoop-974
Repository: sqoop-sqoop2
Description
-------
This patch adds support for stage table to Sqoop2.
Diffs
-----
common/src/main/java/org/apache/sqoop/submission/SubmissionStatus.java e2da8f5
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorError.java 671bb4a
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 75cf9d9
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportDestroyer.java 588e236
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportInitializer.java 7212843
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportDestroyer.java 2cf07fe
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcValidator.java 4e24517
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ExportTableForm.java a311c06
connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-resources.properties 0950e32
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java PRE-CREATION
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExportInitializer.java f83aaa2
core/src/main/java/org/apache/sqoop/framework/JobManager.java e052584
spi/src/main/java/org/apache/sqoop/job/etl/Destroyer.java 149ad2c
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableExportTest.java 436fdfb
Diff: https://reviews.apache.org/r/14022/diff/
Testing
-------
Unit tests as well as integration tests have been added.
All the tests pass.
Feature was manually tested.
Thanks,
Raghav Gautam
Re: Review Request 14022: SQOOP-974 Support for stage table
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14022/#review25988
-----------------------------------------------------------
Hi Raghav,
thank you very much for working on this feature! I've done a high level review and I do have couple of questions/suggestions:
core/src/main/java/org/apache/sqoop/framework/JobManager.java
<https://reviews.apache.org/r/14022/#comment50742>
In Sqoop2 mapreduce job and the server execution are decoupled. One can stop server and the job should still finish and do everything as necessary. As a result destroy callbacks should be called from within the OutputCommitter not from server itself.
spi/src/main/java/org/apache/sqoop/job/etl/Destroyer.java
<https://reviews.apache.org/r/14022/#comment50740>
I do not quite understand why we are introducing a new callback when we already have the destroy callback in place? It seems to me that they are meant to do the same job.
Also please note that the model classes are meant to be abstraction for the server, connectors should always deal with the configuration objects.
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableExportTest.java
<https://reviews.apache.org/r/14022/#comment50741>
Would you mind putting the integration test into standalone class/file?
Jarcec
- Jarek Cecho
On Sept. 7, 2013, 12:18 a.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14022/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2013, 12:18 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: sqoop-974
> https://issues.apache.org/jira/browse/sqoop-974
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> This patch adds support for stage table to Sqoop2.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/submission/SubmissionStatus.java e2da8f5
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorError.java 671bb4a
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 75cf9d9
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportDestroyer.java 588e236
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportInitializer.java 7212843
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportDestroyer.java 2cf07fe
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcValidator.java 4e24517
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ExportTableForm.java a311c06
> connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-resources.properties 0950e32
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java PRE-CREATION
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExportInitializer.java f83aaa2
> core/src/main/java/org/apache/sqoop/framework/JobManager.java e052584
> spi/src/main/java/org/apache/sqoop/job/etl/Destroyer.java 149ad2c
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableExportTest.java 436fdfb
>
> Diff: https://reviews.apache.org/r/14022/diff/
>
>
> Testing
> -------
>
> Unit tests as well as integration tests have been added.
> All the tests pass.
> Feature was manually tested.
>
>
> Thanks,
>
> Raghav Gautam
>
>
Re: Review Request 14022: SQOOP-974 Support for stage table
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14022/#review26015
-----------------------------------------------------------
Hi Raghav,
thank you very much for moving the functionality to the destroyer! I do have couple of questions, mostly nits:
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java
<https://reviews.apache.org/r/14022/#comment50778>
Nit: Wouldn't be simpler to call LOG.warn(message, exception) rather than building the string manually?
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportDestroyer.java
<https://reviews.apache.org/r/14022/#comment50782>
Nit: Unused imports
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportDestroyer.java
<https://reviews.apache.org/r/14022/#comment50783>
Nit: Unused imports
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java
<https://reviews.apache.org/r/14022/#comment50779>
Nit: Seems as not necessary any more.
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExportInitializer.java
<https://reviews.apache.org/r/14022/#comment50780>
Nit: This change do not seem to be necessary.
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedExportTest.java
<https://reviews.apache.org/r/14022/#comment50781>
Please move this file to "exports" sub module.
Jarcec
- Jarek Cecho
On Sept. 9, 2013, 9:41 p.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14022/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2013, 9:41 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: sqoop-974
> https://issues.apache.org/jira/browse/sqoop-974
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> This patch adds support for stage table to Sqoop2.
>
>
> Diffs
> -----
>
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorError.java 671bb4a
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 75cf9d9
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportDestroyer.java 588e236
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportInitializer.java 7212843
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportDestroyer.java 2cf07fe
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcValidator.java 4e24517
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ExportTableForm.java a311c06
> connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-resources.properties 0950e32
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java PRE-CREATION
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExportInitializer.java f83aaa2
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedExportTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14022/diff/
>
>
> Testing
> -------
>
> Unit tests as well as integration tests have been added.
> All the tests pass.
> Feature was manually tested.
>
>
> Thanks,
>
> Raghav Gautam
>
>
Re: Review Request 14022: SQOOP-974 Support for stage table
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14022/#review26037
-----------------------------------------------------------
Ship it!
Ship It!
- Jarek Cecho
On Sept. 10, 2013, 6:39 p.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14022/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2013, 6:39 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: sqoop-974
> https://issues.apache.org/jira/browse/sqoop-974
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> This patch adds support for stage table to Sqoop2.
>
>
> Diffs
> -----
>
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorError.java 671bb4a
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 75cf9d9
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportDestroyer.java 588e236
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportInitializer.java 7212843
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcValidator.java 4e24517
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ExportTableForm.java a311c06
> connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-resources.properties 0950e32
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java PRE-CREATION
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExportInitializer.java f83aaa2
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/exports/TableStagedExportTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14022/diff/
>
>
> Testing
> -------
>
> Unit tests as well as integration tests have been added.
> All the tests pass.
> Feature was manually tested.
>
>
> Thanks,
>
> Raghav Gautam
>
>
Re: Review Request 14022: SQOOP-974 Support for stage table
Posted by Raghav Gautam <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14022/
-----------------------------------------------------------
(Updated Sept. 10, 2013, 11:39 a.m.)
Review request for Sqoop.
Changes
-------
Updated the patch according to the review comments.
Bugs: sqoop-974
https://issues.apache.org/jira/browse/sqoop-974
Repository: sqoop-sqoop2
Description
-------
This patch adds support for stage table to Sqoop2.
Diffs (updated)
-----
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorError.java 671bb4a
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 75cf9d9
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportDestroyer.java 588e236
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportInitializer.java 7212843
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcValidator.java 4e24517
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ExportTableForm.java a311c06
connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-resources.properties 0950e32
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java PRE-CREATION
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExportInitializer.java f83aaa2
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/exports/TableStagedExportTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/14022/diff/
Testing
-------
Unit tests as well as integration tests have been added.
All the tests pass.
Feature was manually tested.
Thanks,
Raghav Gautam
Re: Review Request 14022: SQOOP-974 Support for stage table
Posted by Raghav Gautam <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14022/
-----------------------------------------------------------
(Updated Sept. 9, 2013, 2:41 p.m.)
Review request for Sqoop.
Changes
-------
Uploading the updated patch.
Bugs: sqoop-974
https://issues.apache.org/jira/browse/sqoop-974
Repository: sqoop-sqoop2
Description
-------
This patch adds support for stage table to Sqoop2.
Diffs (updated)
-----
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorError.java 671bb4a
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 75cf9d9
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportDestroyer.java 588e236
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportInitializer.java 7212843
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportDestroyer.java 2cf07fe
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcValidator.java 4e24517
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ExportTableForm.java a311c06
connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-resources.properties 0950e32
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutorTest.java PRE-CREATION
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExportInitializer.java f83aaa2
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedExportTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/14022/diff/
Testing
-------
Unit tests as well as integration tests have been added.
All the tests pass.
Feature was manually tested.
Thanks,
Raghav Gautam