- Fero
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64715/#review194418 ----------------------------------------------------------- On Dec. 27, 2017, 3:36 p.m., Fero Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64715/ > ----------------------------------------------------------- > > (Updated Dec. 27, 2017, 3:36 p.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/tool/ImportAllTablesTool.java d6d9f604 > src/test/org/apache/sqoop/TestSqoopOptions.java 6d55c337 > > > Diff: https://reviews.apache.org/r/64715/diff/2/ > > > 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 > >
