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:23:58 GMT

This is an automatically generated e-mail. To reply, visit:

(Updated Jan. 4, 2018, 2:23 p.m.)

Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.

Bugs: SQOOP-3241

Repository: sqoop-trunk



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

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

Changes: https://reviews.apache.org/r/64715/diff/3-4/


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.


Fero Szabo

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