Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Sqoop, mail # dev - Review Request 14022: SQOOP-974 Support for stage table


Copy link to this message
-
Re: Review Request 14022: SQOOP-974 Support for stage table
Jarek Cecho 2013-09-09, 09:16

-----------------------------------------------------------
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
>
>