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