You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by rj...@gmail.com on 2013/04/30 21:56:28 UTC
Review Request: Sqoop2: Introduce synchronous job submission to Client API
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10869/
-----------------------------------------------------------
Review request for Sqoop.
Description
-------
Synchronous job submission to Client API.
This addresses bug SQOOP-985.
https://issues.apache.org/jira/browse/SQOOP-985
Diffs
-----
client/src/main/java/org/apache/sqoop/client/SqoopClient.java f9137bb
client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java f68ac11
docs/src/site/sphinx/ClientAPI.rst a9d39fb
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableImportTest.java adcfbaf
Diff: https://reviews.apache.org/r/10869/diff/
Testing
-------
Done
Thanks,
vasanthkumar
Re: Review Request: Sqoop2: Introduce synchronous job submission to Client
API
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10869/#review20116
-----------------------------------------------------------
Hi Vasanth,
thank you very much for incorporating my suggestions. I do have couple of notes:
client/src/main/java/org/apache/sqoop/client/SqoopClient.java
<https://reviews.apache.org/r/10869/#comment41364>
Can we introduce new error code explaining that the poll time can't be negative?
client/src/main/java/org/apache/sqoop/client/SqoopClient.java
<https://reviews.apache.org/r/10869/#comment41365>
I think that returning immediately will change the behaviour of this method from synchronous to asynchronous, which might be quite confusing for our users. What about just calling the callbacks if the object is not null? This way the method will still be synchronous but the callbacks will be still optional.
client/src/main/java/org/apache/sqoop/client/SubmissionCallback.java
<https://reviews.apache.org/r/10869/#comment41366>
I was thinking a bit about the callback interface and I think that it would be useful to have it more powerful. What about introducing following method?
* submitted(MSubmission)
* updated(MSubmission)
* finished(MSubmission)
Jarcec
- Jarek Cecho
On May 3, 2013, 1:18 p.m., vasanthkumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10869/
> -----------------------------------------------------------
>
> (Updated May 3, 2013, 1:18 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Synchronous job submission to Client API.
>
>
> This addresses bug SQOOP-985.
> https://issues.apache.org/jira/browse/SQOOP-985
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/sqoop/client/SqoopClient.java f9137bb
> client/src/main/java/org/apache/sqoop/client/SubmissionCallback.java PRE-CREATION
> client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java f68ac11
>
> Diff: https://reviews.apache.org/r/10869/diff/
>
>
> Testing
> -------
>
> Done
>
>
> Thanks,
>
> vasanthkumar
>
>
Re: Review Request: Sqoop2: Introduce synchronous job submission to Client
API
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10869/#review20765
-----------------------------------------------------------
Ship it!
Ship It!
- Jarek Cecho
On May 3, 2013, 10:14 p.m., vasanthkumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10869/
> -----------------------------------------------------------
>
> (Updated May 3, 2013, 10:14 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Synchronous job submission to Client API.
>
>
> This addresses bug SQOOP-985.
> https://issues.apache.org/jira/browse/SQOOP-985
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/sqoop/client/SqoopClient.java f9137bb
> client/src/main/java/org/apache/sqoop/client/SubmissionCallback.java PRE-CREATION
> client/src/main/java/org/apache/sqoop/client/core/ClientError.java 179dc55
> client/src/main/java/org/apache/sqoop/client/core/Constants.java a48857e
> client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java f68ac11
> docs/src/site/sphinx/ClientAPI.rst a9d39fb
>
> Diff: https://reviews.apache.org/r/10869/diff/
>
>
> Testing
> -------
>
> Done
>
>
> Thanks,
>
> vasanthkumar
>
>
Re: Review Request: Sqoop2: Introduce synchronous job submission to Client
API
Posted by rj...@gmail.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10869/
-----------------------------------------------------------
(Updated May 3, 2013, 10:14 p.m.)
Review request for Sqoop.
Changes
-------
Implemented Jarek's comments. Update Client API document.
Description
-------
Synchronous job submission to Client API.
This addresses bug SQOOP-985.
https://issues.apache.org/jira/browse/SQOOP-985
Diffs (updated)
-----
client/src/main/java/org/apache/sqoop/client/SqoopClient.java f9137bb
client/src/main/java/org/apache/sqoop/client/SubmissionCallback.java PRE-CREATION
client/src/main/java/org/apache/sqoop/client/core/ClientError.java 179dc55
client/src/main/java/org/apache/sqoop/client/core/Constants.java a48857e
client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java f68ac11
docs/src/site/sphinx/ClientAPI.rst a9d39fb
Diff: https://reviews.apache.org/r/10869/diff/
Testing
-------
Done
Thanks,
vasanthkumar
Re: Review Request: Sqoop2: Introduce synchronous job submission to Client
API
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10869/#review20147
-----------------------------------------------------------
Hi Vasanth,
thank you for incorporating all my comments. I do have couple of more:
client/src/main/java/org/apache/sqoop/client/SqoopClient.java
<https://reviews.apache.org/r/10869/#comment41397>
Please use CamelCase for naming the Enum, so that we're consistent with rest of the code base.
client/src/main/java/org/apache/sqoop/client/SqoopClient.java
<https://reviews.apache.org/r/10869/#comment41393>
I'm thinking what will happen when the pollTime will be zero? Maybe it would be worth changing the condition to pollTime <= 0?
client/src/main/java/org/apache/sqoop/client/SqoopClient.java
<https://reviews.apache.org/r/10869/#comment41395>
By supplying the pollTime parameter user is effectively driving how many HTTP request will be generated or what will be the minimal delay of getting update from Server. Having said that I think we should not override it even when user is not interested in getting any callbacks called.
client/src/main/java/org/apache/sqoop/client/SqoopClient.java
<https://reviews.apache.org/r/10869/#comment41401>
Nit: "} else {"
client/src/main/java/org/apache/sqoop/client/SqoopClient.java
<https://reviews.apache.org/r/10869/#comment41399>
Can you provide Java doc to this method describing what this method does?
Also I would suggest to rename the method as it do not seem to be setting any submission status. Perhaps submissionCallback()?
- Jarek Cecho
On May 3, 2013, 8:31 p.m., vasanthkumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10869/
> -----------------------------------------------------------
>
> (Updated May 3, 2013, 8:31 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Synchronous job submission to Client API.
>
>
> This addresses bug SQOOP-985.
> https://issues.apache.org/jira/browse/SQOOP-985
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/sqoop/client/SqoopClient.java f9137bb
> client/src/main/java/org/apache/sqoop/client/SubmissionCallback.java PRE-CREATION
> client/src/main/java/org/apache/sqoop/client/core/ClientError.java 179dc55
> client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java f68ac11
>
> Diff: https://reviews.apache.org/r/10869/diff/
>
>
> Testing
> -------
>
> Done
>
>
> Thanks,
>
> vasanthkumar
>
>
Re: Review Request: Sqoop2: Introduce synchronous job submission to Client
API
Posted by rj...@gmail.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10869/
-----------------------------------------------------------
(Updated May 3, 2013, 8:31 p.m.)
Review request for Sqoop.
Changes
-------
Implemented Jarek's suggestion. New startSubmission method returns submission status, I added this because when callback is set to null then user may not know final status. So after completing exection returns final finish status of submission or any submission error status.
Uploaded draft patch. Still requires document update. Kindly verify.
Description
-------
Synchronous job submission to Client API.
This addresses bug SQOOP-985.
https://issues.apache.org/jira/browse/SQOOP-985
Diffs (updated)
-----
client/src/main/java/org/apache/sqoop/client/SqoopClient.java f9137bb
client/src/main/java/org/apache/sqoop/client/SubmissionCallback.java PRE-CREATION
client/src/main/java/org/apache/sqoop/client/core/ClientError.java 179dc55
client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java f68ac11
Diff: https://reviews.apache.org/r/10869/diff/
Testing
-------
Done
Thanks,
vasanthkumar
Re: Review Request: Sqoop2: Introduce synchronous job submission to Client
API
Posted by rj...@gmail.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10869/
-----------------------------------------------------------
(Updated May 3, 2013, 1:18 p.m.)
Review request for Sqoop.
Changes
-------
Introduced callback interface. Implemented in submission start function and prints the submisssion status. Kindly verify.
Description
-------
Synchronous job submission to Client API.
This addresses bug SQOOP-985.
https://issues.apache.org/jira/browse/SQOOP-985
Diffs (updated)
-----
client/src/main/java/org/apache/sqoop/client/SqoopClient.java f9137bb
client/src/main/java/org/apache/sqoop/client/SubmissionCallback.java PRE-CREATION
client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java f68ac11
Diff: https://reviews.apache.org/r/10869/diff/
Testing
-------
Done
Thanks,
vasanthkumar
Re: Review Request: Sqoop2: Introduce synchronous job submission to Client
API
Posted by Jarek Cecho <ja...@apache.org>.
> On May 2, 2013, 12:50 a.m., Jarek Cecho wrote:
> > client/src/main/java/org/apache/sqoop/client/SqoopClient.java, line 355
> > <https://reviews.apache.org/r/10869/diff/2/?file=286264#file286264line355>
> >
> > What about rather then creating one method that can do both synchronous and asynchronous job submission create method startSubmission(long jid) for synchronous mode and startSubmission(long jid, callback, long polltime) for asynchronous mode? I think that there is no need for the Long object, I would personally require caller to specify a poll period if synchronous mode is required. I would also like to expose callback that will be called every poll period with updated MSubmission object so that user code can react on that. We can easily reuse such callback in client shell to print out updated status.
>
> vasanthkumar wrote:
> Hi Jarcec,
> Ok. can we have startSubmission(long jid, callback, long polltime) for synchronous? Given asynchoronous. Need to create seperate class Callback or callback inner class of SqoopClient? We can have negative value for poll period and updated to client only after job finish.
Those are really good questions Vasanth! I think that having negative value for poll period is invalid and thus I would throw an exception. We could potentially allow the callback to be null and in such case we would not be calling user defined function at all. Do you think that it would be beneficial? If you do not mind, I would prefer to have the callback interface as a separate interface so that we could potentially reuse somewhere else if needed (I'm not sure if that will be the case though).
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10869/#review20042
-----------------------------------------------------------
On April 30, 2013, 7:56 p.m., vasanthkumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10869/
> -----------------------------------------------------------
>
> (Updated April 30, 2013, 7:56 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Synchronous job submission to Client API.
>
>
> This addresses bug SQOOP-985.
> https://issues.apache.org/jira/browse/SQOOP-985
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/sqoop/client/SqoopClient.java f9137bb
> client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java f68ac11
> docs/src/site/sphinx/ClientAPI.rst a9d39fb
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableImportTest.java adcfbaf
>
> Diff: https://reviews.apache.org/r/10869/diff/
>
>
> Testing
> -------
>
> Done
>
>
> Thanks,
>
> vasanthkumar
>
>
Re: Review Request: Sqoop2: Introduce synchronous job submission to Client
API
Posted by rj...@gmail.com.
> On May 2, 2013, 12:50 a.m., Jarek Cecho wrote:
> > client/src/main/java/org/apache/sqoop/client/SqoopClient.java, line 355
> > <https://reviews.apache.org/r/10869/diff/2/?file=286264#file286264line355>
> >
> > What about rather then creating one method that can do both synchronous and asynchronous job submission create method startSubmission(long jid) for synchronous mode and startSubmission(long jid, callback, long polltime) for asynchronous mode? I think that there is no need for the Long object, I would personally require caller to specify a poll period if synchronous mode is required. I would also like to expose callback that will be called every poll period with updated MSubmission object so that user code can react on that. We can easily reuse such callback in client shell to print out updated status.
Hi Jarcec,
Ok. can we have startSubmission(long jid, callback, long polltime) for synchronous? Given asynchoronous. Need to create seperate class Callback or callback inner class of SqoopClient? We can have negative value for poll period and updated to client only after job finish.
- vasanthkumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10869/#review20042
-----------------------------------------------------------
On April 30, 2013, 7:56 p.m., vasanthkumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10869/
> -----------------------------------------------------------
>
> (Updated April 30, 2013, 7:56 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Synchronous job submission to Client API.
>
>
> This addresses bug SQOOP-985.
> https://issues.apache.org/jira/browse/SQOOP-985
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/sqoop/client/SqoopClient.java f9137bb
> client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java f68ac11
> docs/src/site/sphinx/ClientAPI.rst a9d39fb
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableImportTest.java adcfbaf
>
> Diff: https://reviews.apache.org/r/10869/diff/
>
>
> Testing
> -------
>
> Done
>
>
> Thanks,
>
> vasanthkumar
>
>
Re: Review Request: Sqoop2: Introduce synchronous job submission to Client
API
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10869/#review20042
-----------------------------------------------------------
Hi Vasanth,
thank you very much for working on this JIRA.
client/src/main/java/org/apache/sqoop/client/SqoopClient.java
<https://reviews.apache.org/r/10869/#comment41296>
What about rather then creating one method that can do both synchronous and asynchronous job submission create method startSubmission(long jid) for synchronous mode and startSubmission(long jid, callback, long polltime) for asynchronous mode? I think that there is no need for the Long object, I would personally require caller to specify a poll period if synchronous mode is required. I would also like to expose callback that will be called every poll period with updated MSubmission object so that user code can react on that. We can easily reuse such callback in client shell to print out updated status.
Jarcec
- Jarek Cecho
On April 30, 2013, 7:56 p.m., vasanthkumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10869/
> -----------------------------------------------------------
>
> (Updated April 30, 2013, 7:56 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Synchronous job submission to Client API.
>
>
> This addresses bug SQOOP-985.
> https://issues.apache.org/jira/browse/SQOOP-985
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/sqoop/client/SqoopClient.java f9137bb
> client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java f68ac11
> docs/src/site/sphinx/ClientAPI.rst a9d39fb
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableImportTest.java adcfbaf
>
> Diff: https://reviews.apache.org/r/10869/diff/
>
>
> Testing
> -------
>
> Done
>
>
> Thanks,
>
> vasanthkumar
>
>