sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Fero Szabo via Review Board <nore...@reviews.apache.org>
Subject Re: Review Request 67524: SQOOP-3333 Change default behavior of the MS SQL connector to non-resilient.
Date Tue, 19 Jun 2018 10:29:10 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message