You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Venkat Ranganathan <n....@live.com> on 2014/02/03 05:28:47 UTC
Review Request 17650: SQOOP-1278: Allow use of uncommitted isolation on
databases that support is an import option
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17650/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-1278
https://issues.apache.org/jira/browse/SQOOP-1278
Repository: sqoop-trunk
Description
-------
Allow the use of read uncommitted isolation on all databases that support it. Oracle does not read uncommitted as a settable option in the JDBC drivers so it will behave as before on Oracle.
Diffs
-----
src/java/org/apache/sqoop/SqoopOptions.java 46e158c
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java b21560e
src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java be942ce
src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java 73ed94e
src/java/org/apache/sqoop/tool/BaseSqoopTool.java 6d6f1ea
src/test/com/cloudera/sqoop/TestSqoopOptions.java 686d398
Diff: https://reviews.apache.org/r/17650/diff/
Testing
-------
One new test to test the option. Explicitly tested with Oracle, DB2 and Postgresql. All unit tests passed
Thanks,
Venkat Ranganathan
Re: Review Request 17650: SQOOP-1278: Allow use of uncommitted isolation on
databases that support is an import option
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17650/#review33637
-----------------------------------------------------------
Ship it!
Ship It!
- Jarek Cecho
On Feb. 4, 2014, 6:22 p.m., Venkat Ranganathan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17650/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 6:22 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1278
> https://issues.apache.org/jira/browse/SQOOP-1278
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Allow the use of read uncommitted isolation on all databases that support it. Oracle does not read uncommitted as a settable option in the JDBC drivers so it will behave as before on Oracle.
>
>
> Diffs
> -----
>
> src/docs/user/common-args.txt 8a017f4
> src/docs/user/import.txt 0db6d97
> src/java/org/apache/sqoop/SqoopOptions.java 46e158c
> src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java b21560e
> src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java be942ce
> src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java 73ed94e
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 6d6f1ea
> src/test/com/cloudera/sqoop/TestSqoopOptions.java 686d398
>
> Diff: https://reviews.apache.org/r/17650/diff/
>
>
> Testing
> -------
>
> One new test to test the option. Explicitly tested with Oracle, DB2 and Postgresql. All unit tests passed
>
>
> Thanks,
>
> Venkat Ranganathan
>
>
Re: Review Request 17650: SQOOP-1278: Allow use of uncommitted isolation on
databases that support is an import option
Posted by Venkat Ranganathan <n....@live.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17650/
-----------------------------------------------------------
(Updated Feb. 4, 2014, 6:22 p.m.)
Review request for Sqoop.
Changes
-------
Thanks for a quick review Jarcec. Incorporated all your suggestions and fixes.
Thanks
Venkat
Bugs: SQOOP-1278
https://issues.apache.org/jira/browse/SQOOP-1278
Repository: sqoop-trunk
Description
-------
Allow the use of read uncommitted isolation on all databases that support it. Oracle does not read uncommitted as a settable option in the JDBC drivers so it will behave as before on Oracle.
Diffs (updated)
-----
src/docs/user/common-args.txt 8a017f4
src/docs/user/import.txt 0db6d97
src/java/org/apache/sqoop/SqoopOptions.java 46e158c
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java b21560e
src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java be942ce
src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java 73ed94e
src/java/org/apache/sqoop/tool/BaseSqoopTool.java 6d6f1ea
src/test/com/cloudera/sqoop/TestSqoopOptions.java 686d398
Diff: https://reviews.apache.org/r/17650/diff/
Testing
-------
One new test to test the option. Explicitly tested with Oracle, DB2 and Postgresql. All unit tests passed
Thanks,
Venkat Ranganathan
Re: Review Request 17650: SQOOP-1278: Allow use of uncommitted isolation on
databases that support is an import option
Posted by Venkat Ranganathan <n....@live.com>.
> On Feb. 4, 2014, 5:09 p.m., Jarek Cecho wrote:
> > Couple of nits:
Thanks a lot for a quick review. I was trying to use a different eclipse bundle and this seems to be little more aggressive in importing packages. Let me try to fix the configuration. Sorry about that.
> On Feb. 4, 2014, 5:09 p.m., Jarek Cecho wrote:
> > src/docs/user/import.txt, lines 296-297
> > <https://reviews.apache.org/r/17650/diff/2/?file=463487#file463487line296>
> >
> > Can we further tweak this by also stating that not all connectors might following this argument?
Sure. Good suggestion.
> On Feb. 4, 2014, 5:09 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/SqoopOptions.java, line 304
> > <https://reviews.apache.org/r/17650/diff/2/?file=463488#file463488line304>
> >
> > I know that Java boolean will be false by default, but would you mind stating the default value in initDefaults() method as well?
Will do.
> On Feb. 4, 2014, 5:09 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java, line 25
> > <https://reviews.apache.org/r/17650/diff/2/?file=463489#file463489line25>
> >
> > Nit: Seems as unnecessary import.
Fixed. Good catch
> On Feb. 4, 2014, 5:09 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java, line 32
> > <https://reviews.apache.org/r/17650/diff/2/?file=463491#file463491line32>
> >
> > Nit: Seems as unnecessary import.
Good catch. Fixed
- Venkat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17650/#review33603
-----------------------------------------------------------
On Feb. 4, 2014, 6:22 p.m., Venkat Ranganathan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17650/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 6:22 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1278
> https://issues.apache.org/jira/browse/SQOOP-1278
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Allow the use of read uncommitted isolation on all databases that support it. Oracle does not read uncommitted as a settable option in the JDBC drivers so it will behave as before on Oracle.
>
>
> Diffs
> -----
>
> src/docs/user/common-args.txt 8a017f4
> src/docs/user/import.txt 0db6d97
> src/java/org/apache/sqoop/SqoopOptions.java 46e158c
> src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java b21560e
> src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java be942ce
> src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java 73ed94e
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 6d6f1ea
> src/test/com/cloudera/sqoop/TestSqoopOptions.java 686d398
>
> Diff: https://reviews.apache.org/r/17650/diff/
>
>
> Testing
> -------
>
> One new test to test the option. Explicitly tested with Oracle, DB2 and Postgresql. All unit tests passed
>
>
> Thanks,
>
> Venkat Ranganathan
>
>
Re: Review Request 17650: SQOOP-1278: Allow use of uncommitted isolation on
databases that support is an import option
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17650/#review33603
-----------------------------------------------------------
Couple of nits:
src/docs/user/import.txt
<https://reviews.apache.org/r/17650/#comment63135>
Can we further tweak this by also stating that not all connectors might following this argument?
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/17650/#comment63137>
I know that Java boolean will be false by default, but would you mind stating the default value in initDefaults() method as well?
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/17650/#comment63136>
Super nit: Unnecessary new line character.
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java
<https://reviews.apache.org/r/17650/#comment63138>
Nit: Seems as unnecessary import.
src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java
<https://reviews.apache.org/r/17650/#comment63139>
Nit: Seems as unnecessary import.
Jarcec
- Jarek Cecho
On Feb. 3, 2014, 8:01 p.m., Venkat Ranganathan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17650/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2014, 8:01 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1278
> https://issues.apache.org/jira/browse/SQOOP-1278
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Allow the use of read uncommitted isolation on all databases that support it. Oracle does not read uncommitted as a settable option in the JDBC drivers so it will behave as before on Oracle.
>
>
> Diffs
> -----
>
> src/docs/user/common-args.txt 8a017f4
> src/docs/user/import.txt 0db6d97
> src/java/org/apache/sqoop/SqoopOptions.java 46e158c
> src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java b21560e
> src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java be942ce
> src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java 73ed94e
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 6d6f1ea
> src/test/com/cloudera/sqoop/TestSqoopOptions.java 686d398
>
> Diff: https://reviews.apache.org/r/17650/diff/
>
>
> Testing
> -------
>
> One new test to test the option. Explicitly tested with Oracle, DB2 and Postgresql. All unit tests passed
>
>
> Thanks,
>
> Venkat Ranganathan
>
>
Re: Review Request 17650: SQOOP-1278: Allow use of uncommitted isolation on
databases that support is an import option
Posted by Venkat Ranganathan <n....@live.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17650/
-----------------------------------------------------------
(Updated Feb. 3, 2014, 8:01 p.m.)
Review request for Sqoop.
Changes
-------
Added comment on import about the new option and update the list of options to include it
Bugs: SQOOP-1278
https://issues.apache.org/jira/browse/SQOOP-1278
Repository: sqoop-trunk
Description
-------
Allow the use of read uncommitted isolation on all databases that support it. Oracle does not read uncommitted as a settable option in the JDBC drivers so it will behave as before on Oracle.
Diffs (updated)
-----
src/docs/user/common-args.txt 8a017f4
src/docs/user/import.txt 0db6d97
src/java/org/apache/sqoop/SqoopOptions.java 46e158c
src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java b21560e
src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java be942ce
src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java 73ed94e
src/java/org/apache/sqoop/tool/BaseSqoopTool.java 6d6f1ea
src/test/com/cloudera/sqoop/TestSqoopOptions.java 686d398
Diff: https://reviews.apache.org/r/17650/diff/
Testing
-------
One new test to test the option. Explicitly tested with Oracle, DB2 and Postgresql. All unit tests passed
Thanks,
Venkat Ranganathan
Re: Review Request 17650: SQOOP-1278: Allow use of uncommitted isolation on
databases that support is an import option
Posted by Jarek Cecho <ja...@apache.org>.
> On Feb. 3, 2014, 4:58 p.m., Jarek Cecho wrote:
> > Hi Venkat,
> > thank you very much for working on this one! Allowing user to set the transaction isolation level seems to be good idea. I'm a bit concerned that the change is only in the DBInputFormat and as a result it won't be utilized by connectors using their own input formats, but I guess that there is not much we can do about that in Sqoop 1. Hopefully we will have better situation in Sqoop 2! Couple of high level notes:
> >
> > 1) Do you think that it would be useful to let user set any transaction level? Right now Sqoop allows only two, so perhaps allowing all of them might be useful?
> > 2) Do you think that it would be useful to set the transaction isolation also for connections created from within the Sqoop process (e.g. while fetching metadata, ...).
> > 3) Would you mind documenting the new option in the user guide?
> >
> > Jarcec
>
> Venkat Ranganathan wrote:
> Good questions Jarcec. Thanks for the quick review also.
>
> 1) While it may seem that it may seem good to allow all isolation levels, the other isolation levels (repeatable read and serializable) have increased transaction isolation requirements and may not be really useful
> 2) Metadata queries can be read committed (internally some DBMSs do use recursive queries for them) and show that should be fine. For those queries using the connection to get the result set metadata (like 1=0 clause query) do not end up in acquiring any locks so should be OK as it is
> 3) Will definitely add doc. Sorry for missing it
>
Thank you for the answers Venkat, makes complete sense to me.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17650/#review33434
-----------------------------------------------------------
On Feb. 3, 2014, 8:01 p.m., Venkat Ranganathan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17650/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2014, 8:01 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1278
> https://issues.apache.org/jira/browse/SQOOP-1278
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Allow the use of read uncommitted isolation on all databases that support it. Oracle does not read uncommitted as a settable option in the JDBC drivers so it will behave as before on Oracle.
>
>
> Diffs
> -----
>
> src/docs/user/common-args.txt 8a017f4
> src/docs/user/import.txt 0db6d97
> src/java/org/apache/sqoop/SqoopOptions.java 46e158c
> src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java b21560e
> src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java be942ce
> src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java 73ed94e
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 6d6f1ea
> src/test/com/cloudera/sqoop/TestSqoopOptions.java 686d398
>
> Diff: https://reviews.apache.org/r/17650/diff/
>
>
> Testing
> -------
>
> One new test to test the option. Explicitly tested with Oracle, DB2 and Postgresql. All unit tests passed
>
>
> Thanks,
>
> Venkat Ranganathan
>
>
Re: Review Request 17650: SQOOP-1278: Allow use of uncommitted isolation on
databases that support is an import option
Posted by Venkat Ranganathan <n....@live.com>.
> On Feb. 3, 2014, 4:58 p.m., Jarek Cecho wrote:
> > Hi Venkat,
> > thank you very much for working on this one! Allowing user to set the transaction isolation level seems to be good idea. I'm a bit concerned that the change is only in the DBInputFormat and as a result it won't be utilized by connectors using their own input formats, but I guess that there is not much we can do about that in Sqoop 1. Hopefully we will have better situation in Sqoop 2! Couple of high level notes:
> >
> > 1) Do you think that it would be useful to let user set any transaction level? Right now Sqoop allows only two, so perhaps allowing all of them might be useful?
> > 2) Do you think that it would be useful to set the transaction isolation also for connections created from within the Sqoop process (e.g. while fetching metadata, ...).
> > 3) Would you mind documenting the new option in the user guide?
> >
> > Jarcec
Good questions Jarcec. Thanks for the quick review also.
1) While it may seem that it may seem good to allow all isolation levels, the other isolation levels (repeatable read and serializable) have increased transaction isolation requirements and may not be really useful
2) Metadata queries can be read committed (internally some DBMSs do use recursive queries for them) and show that should be fine. For those queries using the connection to get the result set metadata (like 1=0 clause query) do not end up in acquiring any locks so should be OK as it is
3) Will definitely add doc. Sorry for missing it
- Venkat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17650/#review33434
-----------------------------------------------------------
On Feb. 3, 2014, 4:28 a.m., Venkat Ranganathan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17650/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2014, 4:28 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1278
> https://issues.apache.org/jira/browse/SQOOP-1278
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Allow the use of read uncommitted isolation on all databases that support it. Oracle does not read uncommitted as a settable option in the JDBC drivers so it will behave as before on Oracle.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/SqoopOptions.java 46e158c
> src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java b21560e
> src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java be942ce
> src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java 73ed94e
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 6d6f1ea
> src/test/com/cloudera/sqoop/TestSqoopOptions.java 686d398
>
> Diff: https://reviews.apache.org/r/17650/diff/
>
>
> Testing
> -------
>
> One new test to test the option. Explicitly tested with Oracle, DB2 and Postgresql. All unit tests passed
>
>
> Thanks,
>
> Venkat Ranganathan
>
>
Re: Review Request 17650: SQOOP-1278: Allow use of uncommitted isolation on
databases that support is an import option
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17650/#review33434
-----------------------------------------------------------
Hi Venkat,
thank you very much for working on this one! Allowing user to set the transaction isolation level seems to be good idea. I'm a bit concerned that the change is only in the DBInputFormat and as a result it won't be utilized by connectors using their own input formats, but I guess that there is not much we can do about that in Sqoop 1. Hopefully we will have better situation in Sqoop 2! Couple of high level notes:
1) Do you think that it would be useful to let user set any transaction level? Right now Sqoop allows only two, so perhaps allowing all of them might be useful?
2) Do you think that it would be useful to set the transaction isolation also for connections created from within the Sqoop process (e.g. while fetching metadata, ...).
3) Would you mind documenting the new option in the user guide?
Jarcec
- Jarek Cecho
On Feb. 3, 2014, 4:28 a.m., Venkat Ranganathan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17650/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2014, 4:28 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1278
> https://issues.apache.org/jira/browse/SQOOP-1278
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Allow the use of read uncommitted isolation on all databases that support it. Oracle does not read uncommitted as a settable option in the JDBC drivers so it will behave as before on Oracle.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/SqoopOptions.java 46e158c
> src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java b21560e
> src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java be942ce
> src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java 73ed94e
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 6d6f1ea
> src/test/com/cloudera/sqoop/TestSqoopOptions.java 686d398
>
> Diff: https://reviews.apache.org/r/17650/diff/
>
>
> Testing
> -------
>
> One new test to test the option. Explicitly tested with Oracle, DB2 and Postgresql. All unit tests passed
>
>
> Thanks,
>
> Venkat Ranganathan
>
>