----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64715/#review194753 -----------------------------------------------------------
src/java/org/apache/sqoop/SqoopOptions.java Line 108 (original) <https://reviews.apache.org/r/64715/#comment273733> Please do not remove this field, it seems to be a feature added by https://issues.apache.org/jira/browse/SQOOP-2333 some plugins may rely on it. src/java/org/apache/sqoop/SqoopOptions.java Lines 891 (patched) <https://reviews.apache.org/r/64715/#comment273736> Copying Integer values is not necessary since Integer is unmodifiable. src/java/org/apache/sqoop/tool/ImportAllTablesTool.java Line 100 (original), 100 (patched) <https://reviews.apache.org/r/64715/#comment273734> This variable is not necessary anymore. src/test/org/apache/sqoop/TestSqoopOptions.java Lines 60 (patched) <https://reviews.apache.org/r/64715/#comment273737> activeTool field does not exist in SqoopOptions. src/test/org/apache/sqoop/TestSqoopOptions.java Lines 66 (patched) <https://reviews.apache.org/r/64715/#comment273738> I think we should modify clone to copy jobStorageDescriptor just to be on the safe side. src/test/org/apache/sqoop/TestSqoopOptions.java Lines 114 (patched) <https://reviews.apache.org/r/64715/#comment273735> I think getting the applicable fields could be extracted in another method. It would make the test more readable. src/test/org/apache/sqoop/TestSqoopOptions.java Lines 152 (patched) <https://reviews.apache.org/r/64715/#comment273739> Please remove System.out.println. src/test/org/apache/sqoop/TestSqoopOptions.java Lines 167 (patched) <https://reviews.apache.org/r/64715/#comment273740> Please remove System.out.println. - Szabolcs Vasas On Jan. 3, 2018, 11:16 a.m., Fero Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64715/ > ----------------------------------------------------------- > > (Updated Jan. 3, 2018, 11:16 a.m.) > > > Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas. > > > Bugs: SQOOP-3241 > https://issues.apache.org/jira/browse/SQOOP-3241 > > > Repository: sqoop-trunk > > > Description > ------- > > SQOOP-3241 > ========== > > TL;DR: The problem is that the ImportAllTablesTool passes the same > SqoopOptions object in every importTable invocation. Since SqoopOptions is > mutable, this can lead to errors. > > The solution: > ------------ > - SqoopOptions already implements Clonable. The solution uses the clone > method to create a copy of SqoopOptions for each invocation. > - I've also added unit tests for the clone function, and > - Introduced a new (test-scoped) dependency, i.e. assertj, because it > contains the isEqualToComparingFieldByFieldRecursively function > > Concerns: > --------- > - The Clonable interface is not recommended to be used by many sources, but > it seems to be the lesser evil here. > - - Since SqoopOptions has more than a hundred fields, a copy constructor > would add a lot of code to be maintained. > - - Implementing a copy constructor either through reflection or through > serialization would add unwanted complexity. > - - The issues with Clonable really arise when there is a class hierarchy; > this won't be a problem for SqoopOptions, as it doesn't really make sense to > extend this class. > - I've just covered two tools with the unit tests, would we benefit from more > coverage? > - The added dependency (please check if the config looks ok), 2.8.0 is an > older version, but this is because Sqoop is using Java 1.7 > > > Diffs > ----- > > ivy.xml 601aa015 > ivy/libraries.properties 2ef04f4f > src/java/org/apache/sqoop/SqoopOptions.java d5fdfba1 > src/java/org/apache/sqoop/tool/ImportAllTablesTool.java d6d9f604 > src/test/org/apache/sqoop/TestSqoopOptions.java 6d55c337 > > > Diff: https://reviews.apache.org/r/64715/diff/3/ > > > Testing > ------- > > unit tests and 3rd party integration tests > > com.cloudera.sqoop.manager.OracleExportTest had an error in the first run, > but passed in the second. It just seems flaky. > > > Thanks, > > Fero Szabo > >
