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/04/16 01:24:41 UTC

Review Request 33246: SQOOP-1803 JobManager and Execution Engine changes: Support for a injecting and pulling out configs and job output in connectors

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33246/
-----------------------------------------------------------

Review request for Sqoop.


Bugs: SQOOP-1803
    https://issues.apache.org/jira/browse/SQOOP-1803


Repository: sqoop-sqoop2


Description
-------

Finalized version of API that will add support to update configuration objects for incremental import.


Diffs
-----

  connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromDestroyer.java 3a783be 
  connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/Destroyer.java b4ab6d7 
  core/src/main/java/org/apache/sqoop/driver/JobManager.java 4044510 
  test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c 

Diff: https://reviews.apache.org/r/33246/diff/


Testing
-------

I've altered JDBC Connector incremental test case to verify the last value - and it's passing.


Thanks,

Jarek Cecho


Re: Review Request 33246: SQOOP-1803 JobManager and Execution Engine changes: Support for a injecting and pulling out configs and job output in connectors

Posted by Jarek Cecho <ja...@apache.org>.

> On April 16, 2015, 1:11 a.m., Abraham Elmahrek wrote:
> > LGTM with just a few thoughts.

Thank you for the feedback Abe!


> On April 16, 2015, 1:11 a.m., Abraham Elmahrek wrote:
> > core/src/main/java/org/apache/sqoop/driver/JobManager.java, line 651
> > <https://reviews.apache.org/r/33246/diff/1/?file=931122#file931122line651>
> >
> >     This might not be true if the was explicitly stopped.

I think that it should be the case even in stop() routine that is serving the stop action from client. The stop() method retrieves the submission from repository and throw an exception if it's not running. Then it stops the job via execution engine call (submissionEngine.stop(mSubmission.getExternalJobId())), but never updates the submission state - for that it calls updateSubmission() method that is suppose to retrieve the failed (killed) state. Hence at the begging of the updateSubmission() method the submission should be in "running" state (even though that we've just killed the job).

https://github.com/apache/sqoop/blob/sqoop2/core/src/main/java/org/apache/sqoop/driver/JobManager.java#L551


- Jarek


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33246/#review80287
-----------------------------------------------------------


On April 15, 2015, 11:24 p.m., Jarek Cecho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33246/
> -----------------------------------------------------------
> 
> (Updated April 15, 2015, 11:24 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1803
>     https://issues.apache.org/jira/browse/SQOOP-1803
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> Finalized version of API that will add support to update configuration objects for incremental import.
> 
> 
> Diffs
> -----
> 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromDestroyer.java 3a783be 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/Destroyer.java b4ab6d7 
>   core/src/main/java/org/apache/sqoop/driver/JobManager.java 4044510 
>   test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c 
> 
> Diff: https://reviews.apache.org/r/33246/diff/
> 
> 
> Testing
> -------
> 
> I've altered JDBC Connector incremental test case to verify the last value - and it's passing.
> 
> 
> Thanks,
> 
> Jarek Cecho
> 
>


Re: Review Request 33246: SQOOP-1803 JobManager and Execution Engine changes: Support for a injecting and pulling out configs and job output in connectors

Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33246/#review80287
-----------------------------------------------------------

Ship it!


LGTM with just a few thoughts.


connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/Destroyer.java
<https://reviews.apache.org/r/33246/#comment130095>

    Add note saying that this will always be ran?



core/src/main/java/org/apache/sqoop/driver/JobManager.java
<https://reviews.apache.org/r/33246/#comment130094>

    fromLink, toLink



core/src/main/java/org/apache/sqoop/driver/JobManager.java
<https://reviews.apache.org/r/33246/#comment130096>

    This might not be true if the was explicitly stopped.


- Abraham Elmahrek


On April 15, 2015, 11:24 p.m., Jarek Cecho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33246/
> -----------------------------------------------------------
> 
> (Updated April 15, 2015, 11:24 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1803
>     https://issues.apache.org/jira/browse/SQOOP-1803
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> Finalized version of API that will add support to update configuration objects for incremental import.
> 
> 
> Diffs
> -----
> 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromDestroyer.java 3a783be 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/Destroyer.java b4ab6d7 
>   core/src/main/java/org/apache/sqoop/driver/JobManager.java 4044510 
>   test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c 
> 
> Diff: https://reviews.apache.org/r/33246/diff/
> 
> 
> Testing
> -------
> 
> I've altered JDBC Connector incremental test case to verify the last value - and it's passing.
> 
> 
> Thanks,
> 
> Jarek Cecho
> 
>


Re: Review Request 33246: SQOOP-1803 JobManager and Execution Engine changes: Support for a injecting and pulling out configs and job output in connectors

Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33246/
-----------------------------------------------------------

(Updated April 16, 2015, 1:20 a.m.)


Review request for Sqoop.


Changes
-------

Incorporated Abe's feedback.


Bugs: SQOOP-1803
    https://issues.apache.org/jira/browse/SQOOP-1803


Repository: sqoop-sqoop2


Description
-------

Finalized version of API that will add support to update configuration objects for incremental import.


Diffs (updated)
-----

  connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromDestroyer.java 3a783be 
  connector/connector-sdk/src/main/java/org/apache/sqoop/job/etl/Destroyer.java b4ab6d7 
  core/src/main/java/org/apache/sqoop/driver/JobManager.java 4044510 
  test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c 

Diff: https://reviews.apache.org/r/33246/diff/


Testing
-------

I've altered JDBC Connector incremental test case to verify the last value - and it's passing.


Thanks,

Jarek Cecho