sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Fero Szabo via Review Board <nore...@reviews.apache.org>
Subject Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import
Date Thu, 04 Jan 2018 14:37:54 GMT

-----------------------------------------------------------
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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message