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