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