> On Dec. 22, 2017, 12:02 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/tool/ImportAllTablesTool.java > > Line 109 (original), 110 (patched) > > <https://reviews.apache.org/r/64715/diff/1/?file=1924003#file1924003line110> > > > > Now that we clone the whole SqoopOptions object this line and the > > comment becomes redundant.
Thanks, removed these. > On Dec. 22, 2017, 12:02 p.m., Szabolcs Vasas wrote: > > src/test/org/apache/sqoop/TestSqoopOptions.java > > Lines 36 (patched) > > <https://reviews.apache.org/r/64715/diff/1/?file=1924004#file1924004line36> > > > > I think this method is a bit complicated. SqoopOptions has setters > > defined why do we need a SqoopTool for parsing it? > > Also there is no need for catching exceptions in this method, if an > > exception is thrown the test will fail anyway. Thanks for your insights! Regarding the Exceptions: I moved these into the declaration. Better clarity. Regarding using the setters only: I used the parseArguments functions to get closer to what a SqoopOptions instance might look like in production. While it is complicated, I think it's better to create it like this because of the improved coverage. The ideal thing would be even not to call the setters at all. My only criteria for a unit test is that it should be reasonably fast (i.e. less than 500ms). Relying on the Tool classes doesn't violate this, thus I prefer to create the objects this way. Do you disagree? Should we follow a different approach for unit tests? (Maybe keep them from touching unrelated functionality i.e. object construction?) > On Dec. 22, 2017, 12:02 p.m., Szabolcs Vasas wrote: > > src/test/org/apache/sqoop/TestSqoopOptions.java > > Lines 105 (patched) > > <https://reviews.apache.org/r/64715/diff/1/?file=1924004#file1924004line105> > > > > The best practice for asserting Throwable types and messages is to use > > the ExpectedException rule. You can see its example usage in > > org.apache.sqoop.tool.ImportToolValidateOptionsTest This knowledge was missing from my repertoire. Much appriciated. - 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 > >
