-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64715/
-----------------------------------------------------------
(Updated Jan. 4, 2018, 2:37 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 (updated)
-----
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/5/
Changes: https://reviews.apache.org/r/64715/diff/4-5/
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
