----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64715/#review194418 -----------------------------------------------------------
Hi Feró, Thank you for this patch and your thorough description! I think the test you added is a good start, I like the idea of using assertj here, but since it is a really crucial part of this change to verify that the clone implementation works as expected I think we should definitely add more coverage, currently I can comment out almost the whole body of org.apache.sqoop.SqoopOptions#clone and TestSqoopOptions still passes. I have also added a few comments below, please take a look at them. Regards, Szabolcs src/java/org/apache/sqoop/tool/ImportAllTablesTool.java Line 109 (original), 110 (patched) <https://reviews.apache.org/r/64715/#comment273186> Now that we clone the whole SqoopOptions object this line and the comment becomes redundant. src/test/org/apache/sqoop/TestSqoopOptions.java Lines 36 (patched) <https://reviews.apache.org/r/64715/#comment273187> 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. src/test/org/apache/sqoop/TestSqoopOptions.java Lines 105 (patched) <https://reviews.apache.org/r/64715/#comment273189> 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 - Szabolcs Vasas On Dec. 19, 2017, 4:49 p.m., Fero Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64715/ > ----------------------------------------------------------- > > (Updated Dec. 19, 2017, 4:49 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/1/ > > > 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 > >
