You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Fero Szabo via Review Board <no...@reviews.apache.org> on 2018/06/11 12:28:07 UTC
Review Request 67524: SQOOP-3333 Change default behavior of the MS SQL
connector to non-resilient.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/
-----------------------------------------------------------
Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
Bugs: SQOOP-3333
https://issues.apache.org/jira/browse/SQOOP-3333
Repository: sqoop-trunk
Description
-------
This change is about changing the default behavior of the MS SQL connectore from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation.
I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well.
Diffs
-----
src/docs/user/connectors.txt 7c540718
src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
Diff: https://reviews.apache.org/r/67524/diff/1/
Testing
-------
unit and 3rd party tests.
ant docs ran succesfully.
manual testing.
Thanks,
Fero Szabo
Re: Review Request 67524: SQOOP-3333 Change default behavior of the
MS SQL connector to non-resilient.
Posted by daniel voros <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/#review204931
-----------------------------------------------------------
Ship it!
Ship It!
- daniel voros
On June 18, 2018, 2:48 p.m., Fero Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67524/
> -----------------------------------------------------------
>
> (Updated June 18, 2018, 2:48 p.m.)
>
>
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3333
> https://issues.apache.org/jira/browse/SQOOP-3333
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> This change is about changing the default behavior of the MS SQL connector from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation where previously present.
>
> I've refactored the context configuration into a separate class.
>
> I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
>
> I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well (queries and tables).
>
> I've also added new tests that cover what classes get loaded in connection with the resilient option. Also, I've refactored SQL Server import tests and added a few more cases for better coverage. (The query import uses a different method and wasn't covered by these tests at all.)
>
>
> Diffs
> -----
>
> src/docs/user/connectors.txt 7c540718
> src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742
> src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
> src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java PRE-CREATION
> src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java c83c2c93
> src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67524/diff/4/
>
>
> Testing
> -------
>
> Added new unit tests for SqlServerConfigurator.
> unit and 3rd party tests.
> ant docs ran succesfully.
> manual testing.
>
>
> Thanks,
>
> Fero Szabo
>
>
Re: Review Request 67524: SQOOP-3333 Change default behavior of the
MS SQL connector to non-resilient.
Posted by Fero Szabo via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/
-----------------------------------------------------------
(Updated June 19, 2018, 10:28 a.m.)
Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
Bugs: SQOOP-3333
https://issues.apache.org/jira/browse/SQOOP-3333
Repository: sqoop-trunk
Description
-------
This change is about changing the default behavior of the MS SQL connector from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation where previously present.
I've refactored the context configuration into a separate class.
I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well (queries and tables).
I've also added new tests that cover what classes get loaded in connection with the resilient option. Also, I've refactored SQL Server import tests and added a few more cases for better coverage. (The query import uses a different method and wasn't covered by these tests at all.)
Diffs (updated)
-----
src/docs/user/connectors.txt 7c540718
src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742
src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java PRE-CREATION
src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java c83c2c93
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java PRE-CREATION
Diff: https://reviews.apache.org/r/67524/diff/5/
Changes: https://reviews.apache.org/r/67524/diff/4-5/
Testing
-------
Added new unit tests for SqlServerConfigurator.
unit and 3rd party tests.
ant docs ran succesfully.
manual testing.
Thanks,
Fero Szabo
Re: Review Request 67524: SQOOP-3333 Change default behavior of the
MS SQL connector to non-resilient.
Posted by Fero Szabo via Review Board <no...@reviews.apache.org>.
> On June 19, 2018, 7:16 a.m., Szabolcs Vasas wrote:
> > src/docs/user/connectors.txt
> > Line 151 (original), 151 (patched)
> > <https://reviews.apache.org/r/67524/diff/4/?file=2041992#file2041992line151>
> >
> > Shouldn't we just say "This will retry"?
Certainly sounds better.
> On June 19, 2018, 7:16 a.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/manager/SQLServerManager.java
> > Line 54 (original), 48 (patched)
> > <https://reviews.apache.org/r/67524/diff/4/?file=2041994#file2041994line54>
> >
> > There is an extra space here, please remove it.
Done
> On June 19, 2018, 7:16 a.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/manager/SQLServerManager.java
> > Line 156 (original)
> > <https://reviews.apache.org/r/67524/diff/4/?file=2041994#file2041994line164>
> >
> > This input format does not seem to be set by formatConfigurator.configureContextForImport(context, splitColumn) even if the splitColumn is null.
Good catch!
This was done because I've deduped parts of the importTable and the importQuery functions and only the former executed the statement you refer to.
> On June 19, 2018, 7:16 a.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/manager/SQLServerManager.java
> > Line 206 (original)
> > <https://reviews.apache.org/r/67524/diff/4/?file=2041994#file2041994line214>
> >
> > Just to double check: in the new implementation you call configureConnectionRecoveryForExport instead of configureConnectionRecoveryForUpdate but that should be fine since in the original version configureConnectionRecoveryForExport and configureConnectionRecoveryForUpdate were duplicates, so you dropped configureConnectionRecoveryForUpdate and kept configureConnectionRecoveryForExport, right?
yes, those two were duplicates
- Fero
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/#review204978
-----------------------------------------------------------
On June 19, 2018, 10:28 a.m., Fero Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67524/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 10:28 a.m.)
>
>
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3333
> https://issues.apache.org/jira/browse/SQOOP-3333
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> This change is about changing the default behavior of the MS SQL connector from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation where previously present.
>
> I've refactored the context configuration into a separate class.
>
> I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
>
> I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well (queries and tables).
>
> I've also added new tests that cover what classes get loaded in connection with the resilient option. Also, I've refactored SQL Server import tests and added a few more cases for better coverage. (The query import uses a different method and wasn't covered by these tests at all.)
>
>
> Diffs
> -----
>
> src/docs/user/connectors.txt 7c540718
> src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742
> src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
> src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java PRE-CREATION
> src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java c83c2c93
> src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67524/diff/5/
>
>
> Testing
> -------
>
> Added new unit tests for SqlServerConfigurator.
> unit and 3rd party tests.
> ant docs ran succesfully.
> manual testing.
>
>
> Thanks,
>
> Fero Szabo
>
>
Re: Review Request 67524: SQOOP-3333 Change default behavior of the
MS SQL connector to non-resilient.
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/#review204978
-----------------------------------------------------------
Hi Feró,
Thank you for submitting this patch, the changes look good, I have left a few minor comments.
src/docs/user/connectors.txt
Line 151 (original), 151 (patched)
<https://reviews.apache.org/r/67524/#comment287826>
Shouldn't we just say "This will retry"?
src/java/org/apache/sqoop/manager/SQLServerManager.java
Line 54 (original), 48 (patched)
<https://reviews.apache.org/r/67524/#comment287827>
There is an extra space here, please remove it.
src/java/org/apache/sqoop/manager/SQLServerManager.java
Lines 105 (patched)
<https://reviews.apache.org/r/67524/#comment287828>
Unnecessary new line.
src/java/org/apache/sqoop/manager/SQLServerManager.java
Line 156 (original)
<https://reviews.apache.org/r/67524/#comment287831>
This input format does not seem to be set by formatConfigurator.configureContextForImport(context, splitColumn) even if the splitColumn is null.
src/java/org/apache/sqoop/manager/SQLServerManager.java
Line 206 (original)
<https://reviews.apache.org/r/67524/#comment287834>
Just to double check: in the new implementation you call configureConnectionRecoveryForExport instead of configureConnectionRecoveryForUpdate but that should be fine since in the original version configureConnectionRecoveryForExport and configureConnectionRecoveryForUpdate were duplicates, so you dropped configureConnectionRecoveryForUpdate and kept configureConnectionRecoveryForExport, right?
src/java/org/apache/sqoop/manager/SQLServerManager.java
Lines 186 (patched)
<https://reviews.apache.org/r/67524/#comment287829>
nit: the else should go to the previous line after the }
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
Lines 58 (patched)
<https://reviews.apache.org/r/67524/#comment287838>
nit: this could be assertThat(inputFormat).isSameAs(SQLServerDBInputFormat.class); as well
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
Lines 117 (patched)
<https://reviews.apache.org/r/67524/#comment287839>
This method has to be public otherwise the test fails.
- Szabolcs Vasas
On June 18, 2018, 2:48 p.m., Fero Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67524/
> -----------------------------------------------------------
>
> (Updated June 18, 2018, 2:48 p.m.)
>
>
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3333
> https://issues.apache.org/jira/browse/SQOOP-3333
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> This change is about changing the default behavior of the MS SQL connector from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation where previously present.
>
> I've refactored the context configuration into a separate class.
>
> I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
>
> I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well (queries and tables).
>
> I've also added new tests that cover what classes get loaded in connection with the resilient option. Also, I've refactored SQL Server import tests and added a few more cases for better coverage. (The query import uses a different method and wasn't covered by these tests at all.)
>
>
> Diffs
> -----
>
> src/docs/user/connectors.txt 7c540718
> src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742
> src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
> src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java PRE-CREATION
> src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java c83c2c93
> src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67524/diff/4/
>
>
> Testing
> -------
>
> Added new unit tests for SqlServerConfigurator.
> unit and 3rd party tests.
> ant docs ran succesfully.
> manual testing.
>
>
> Thanks,
>
> Fero Szabo
>
>
Re: Review Request 67524: SQOOP-3333 Change default behavior of the
MS SQL connector to non-resilient.
Posted by Fero Szabo via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/
-----------------------------------------------------------
(Updated June 18, 2018, 2:48 p.m.)
Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
Bugs: SQOOP-3333
https://issues.apache.org/jira/browse/SQOOP-3333
Repository: sqoop-trunk
Description
-------
This change is about changing the default behavior of the MS SQL connector from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation where previously present.
I've refactored the context configuration into a separate class.
I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well (queries and tables).
I've also added new tests that cover what classes get loaded in connection with the resilient option. Also, I've refactored SQL Server import tests and added a few more cases for better coverage. (The query import uses a different method and wasn't covered by these tests at all.)
Diffs (updated)
-----
src/docs/user/connectors.txt 7c540718
src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742
src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java PRE-CREATION
src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java c83c2c93
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java PRE-CREATION
Diff: https://reviews.apache.org/r/67524/diff/4/
Changes: https://reviews.apache.org/r/67524/diff/3-4/
Testing
-------
Added new unit tests for SqlServerConfigurator.
unit and 3rd party tests.
ant docs ran succesfully.
manual testing.
Thanks,
Fero Szabo
Re: Review Request 67524: SQOOP-3333 Change default behavior of the
MS SQL connector to non-resilient.
Posted by Fero Szabo via Review Board <no...@reviews.apache.org>.
> On June 18, 2018, 12:35 p.m., daniel voros wrote:
> > Hi Fero,
> >
> > Thank you for taking care of this! I think it's always a good idea to avoid these nagating options. I've posted a few minor issues/questions.
> >
> > Regards,
> > Daniel
Thanks for the review! I've modified the patch based on your findings.
> On June 18, 2018, 12:35 p.m., daniel voros wrote:
> > src/java/org/apache/sqoop/manager/ExportJobContext.java
> > Lines 38 (patched)
> > <https://reviews.apache.org/r/67524/diff/3/?file=2041669#file2041669line38>
> >
> > This new constructor is always called with outputFormatClass=null now. Are you planning on using this later?
removed the unused constructor and modified the patch. I just created it thinking I'd use it eventually...
> On June 18, 2018, 12:35 p.m., daniel voros wrote:
> > src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java
> > Lines 34 (patched)
> > <https://reviews.apache.org/r/67524/diff/3/?file=2041671#file2041671line34>
> >
> > *"to be resilient"?
thx for catching this
> On June 18, 2018, 12:35 p.m., daniel voros wrote:
> > src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java
> > Lines 115 (patched)
> > <https://reviews.apache.org/r/67524/diff/3/?file=2041671#file2041671line115>
> >
> > Could you please add some javadoc about the return value?
Good idea, I just did.
> On June 18, 2018, 12:35 p.m., daniel voros wrote:
> > src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
> > Lines 119 (patched)
> > <https://reviews.apache.org/r/67524/diff/3/?file=2041673#file2041673line119>
> >
> > Could this be a @Before method since it's called from every TC?
Done.
- Fero
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/#review204918
-----------------------------------------------------------
On June 18, 2018, 2:48 p.m., Fero Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67524/
> -----------------------------------------------------------
>
> (Updated June 18, 2018, 2:48 p.m.)
>
>
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3333
> https://issues.apache.org/jira/browse/SQOOP-3333
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> This change is about changing the default behavior of the MS SQL connector from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation where previously present.
>
> I've refactored the context configuration into a separate class.
>
> I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
>
> I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well (queries and tables).
>
> I've also added new tests that cover what classes get loaded in connection with the resilient option. Also, I've refactored SQL Server import tests and added a few more cases for better coverage. (The query import uses a different method and wasn't covered by these tests at all.)
>
>
> Diffs
> -----
>
> src/docs/user/connectors.txt 7c540718
> src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742
> src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
> src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java PRE-CREATION
> src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java c83c2c93
> src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67524/diff/4/
>
>
> Testing
> -------
>
> Added new unit tests for SqlServerConfigurator.
> unit and 3rd party tests.
> ant docs ran succesfully.
> manual testing.
>
>
> Thanks,
>
> Fero Szabo
>
>
Re: Review Request 67524: SQOOP-3333 Change default behavior of the
MS SQL connector to non-resilient.
Posted by daniel voros <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/#review204918
-----------------------------------------------------------
Hi Fero,
Thank you for taking care of this! I think it's always a good idea to avoid these nagating options. I've posted a few minor issues/questions.
Regards,
Daniel
src/docs/user/connectors.txt
Lines 154 (patched)
<https://reviews.apache.org/r/67524/#comment287716>
*and in ascending order?
src/java/org/apache/sqoop/manager/ExportJobContext.java
Lines 38 (patched)
<https://reviews.apache.org/r/67524/#comment287720>
This new constructor is always called with outputFormatClass=null now. Are you planning on using this later?
src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java
Lines 34 (patched)
<https://reviews.apache.org/r/67524/#comment287717>
*"to be resilient"?
src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java
Lines 115 (patched)
<https://reviews.apache.org/r/67524/#comment287719>
Could you please add some javadoc about the return value?
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
Lines 119 (patched)
<https://reviews.apache.org/r/67524/#comment287718>
Could this be a @Before method since it's called from every TC?
- daniel voros
On June 18, 2018, 10:25 a.m., Fero Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67524/
> -----------------------------------------------------------
>
> (Updated June 18, 2018, 10:25 a.m.)
>
>
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3333
> https://issues.apache.org/jira/browse/SQOOP-3333
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> This change is about changing the default behavior of the MS SQL connector from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation where previously present.
>
> I've refactored the context configuration into a separate class.
>
> I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
>
> I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well (queries and tables).
>
> I've also added new tests that cover what classes get loaded in connection with the resilient option. Also, I've refactored SQL Server import tests and added a few more cases for better coverage. (The query import uses a different method and wasn't covered by these tests at all.)
>
>
> Diffs
> -----
>
> src/docs/user/connectors.txt 7c540718
> src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742
> src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
> src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java PRE-CREATION
> src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java c83c2c93
> src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67524/diff/3/
>
>
> Testing
> -------
>
> Added new unit tests for SqlServerConfigurator.
> unit and 3rd party tests.
> ant docs ran succesfully.
> manual testing.
>
>
> Thanks,
>
> Fero Szabo
>
>
Re: Review Request 67524: SQOOP-3333 Change default behavior of the
MS SQL connector to non-resilient.
Posted by Fero Szabo via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/#review204926
-----------------------------------------------------------
src/docs/user/connectors.txt
Lines 154 (patched)
<https://reviews.apache.org/r/67524/#comment287734>
sounded fine for me as is, but can fix :)
src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java
Lines 34 (patched)
<https://reviews.apache.org/r/67524/#comment287737>
great catch!
src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java
Lines 115 (patched)
<https://reviews.apache.org/r/67524/#comment287736>
Sure, make sense
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
Lines 119 (patched)
<https://reviews.apache.org/r/67524/#comment287735>
make sense
- Fero Szabo
On June 18, 2018, 10:25 a.m., Fero Szabo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67524/
> -----------------------------------------------------------
>
> (Updated June 18, 2018, 10:25 a.m.)
>
>
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3333
> https://issues.apache.org/jira/browse/SQOOP-3333
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> This change is about changing the default behavior of the MS SQL connector from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation where previously present.
>
> I've refactored the context configuration into a separate class.
>
> I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
>
> I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well (queries and tables).
>
> I've also added new tests that cover what classes get loaded in connection with the resilient option. Also, I've refactored SQL Server import tests and added a few more cases for better coverage. (The query import uses a different method and wasn't covered by these tests at all.)
>
>
> Diffs
> -----
>
> src/docs/user/connectors.txt 7c540718
> src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742
> src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
> src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java PRE-CREATION
> src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java c83c2c93
> src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67524/diff/3/
>
>
> Testing
> -------
>
> Added new unit tests for SqlServerConfigurator.
> unit and 3rd party tests.
> ant docs ran succesfully.
> manual testing.
>
>
> Thanks,
>
> Fero Szabo
>
>
Re: Review Request 67524: SQOOP-3333 Change default behavior of the
MS SQL connector to non-resilient.
Posted by Fero Szabo via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/
-----------------------------------------------------------
(Updated June 18, 2018, 10:25 a.m.)
Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
Changes
-------
Added missing Apache header and renamed new class.
Bugs: SQOOP-3333
https://issues.apache.org/jira/browse/SQOOP-3333
Repository: sqoop-trunk
Description
-------
This change is about changing the default behavior of the MS SQL connector from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation where previously present.
I've refactored the context configuration into a separate class.
I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well (queries and tables).
I've also added new tests that cover what classes get loaded in connection with the resilient option. Also, I've refactored SQL Server import tests and added a few more cases for better coverage. (The query import uses a different method and wasn't covered by these tests at all.)
Diffs (updated)
-----
src/docs/user/connectors.txt 7c540718
src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742
src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java PRE-CREATION
src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java c83c2c93
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java PRE-CREATION
Diff: https://reviews.apache.org/r/67524/diff/3/
Changes: https://reviews.apache.org/r/67524/diff/2-3/
Testing
-------
Added new unit tests for SqlServerConfigurator.
unit and 3rd party tests.
ant docs ran succesfully.
manual testing.
Thanks,
Fero Szabo
Re: Review Request 67524: SQOOP-3333 Change default behavior of the
MS SQL connector to non-resilient.
Posted by Fero Szabo via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/
-----------------------------------------------------------
(Updated June 18, 2018, 9:26 a.m.)
Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
Bugs: SQOOP-3333
https://issues.apache.org/jira/browse/SQOOP-3333
Repository: sqoop-trunk
Description (updated)
-------
This change is about changing the default behavior of the MS SQL connectore from resilient to non-resilient. I was aiming for the fewest possible modifications while also removed double negation.
I've also changed the documentation of the non-resilient flag and added a note about the implicit requirement of the feature (that the split-by column has to be unique and ordered in ascending order).
I plan to expand the documentation more in SQOOP-3332, as the (now named) resilient flag works not just for export, but import as well.
I've also added new tests that cover what classes get loaded in connection with the resilient option. Also, I've refactored SQL Server import tests and added a few more cases for better coverage. (The query import uses a different method and wasn't covered by these tests at all.)
Diffs (updated)
-----
src/docs/user/connectors.txt 7c540718
src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742
src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f
src/java/org/apache/sqoop/manager/SqlServerManagerFormatConfigurator.java PRE-CREATION
src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java c83c2c93
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerConfigurator.java PRE-CREATION
Diff: https://reviews.apache.org/r/67524/diff/2/
Changes: https://reviews.apache.org/r/67524/diff/1-2/
Testing
-------
unit and 3rd party tests.
ant docs ran succesfully.
manual testing.
Thanks,
Fero Szabo