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