Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Szabolcs Vasas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64715/#review194860
---


Ship it!




Ship It!

- Szabolcs Vasas


On Jan. 4, 2018, 2:37 p.m., Fero Szabo wrote:
> 
> ---
> 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
> -
> 
>   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/
> 
> 
> 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
> 
>

Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Boglarka Egyed]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64715/#review194859
---


Ship it!




Hi Fero,

Thanks for this fix!

Your change looks good to me, also the unit and 3rd party tests pass with your 
patch.

Cheers,
Bogi

- Boglarka Egyed


On Jan. 4, 2018, 2:37 p.m., Fero Szabo wrote:
> 
> ---
> 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
> -
> 
>   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/
> 
> 
> 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
> 
>

Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Fero Szabo via Review Board]

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

Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64715/
---

(Updated Jan. 4, 2018, 2:23 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/4/

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


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

Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Szabolcs Vasas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64715/#review194753
---




src/java/org/apache/sqoop/SqoopOptions.java
Line 108 (original)


Please do not remove this field, it seems to be a feature added by 
https://issues.apache.org/jira/browse/SQOOP-2333 some plugins may rely on it.



src/java/org/apache/sqoop/SqoopOptions.java
Lines 891 (patched)


Copying Integer values is not necessary since Integer is unmodifiable.



src/java/org/apache/sqoop/tool/ImportAllTablesTool.java
Line 100 (original), 100 (patched)


This variable is not necessary anymore.



src/test/org/apache/sqoop/TestSqoopOptions.java
Lines 60 (patched)


activeTool field does not exist in SqoopOptions.



src/test/org/apache/sqoop/TestSqoopOptions.java
Lines 66 (patched)


I think we should modify clone to copy jobStorageDescriptor just to be on 
the safe side.



src/test/org/apache/sqoop/TestSqoopOptions.java
Lines 114 (patched)


I think getting the applicable fields could be extracted in another method. 
It would make the test more readable.



src/test/org/apache/sqoop/TestSqoopOptions.java
Lines 152 (patched)


Please remove System.out.println.



src/test/org/apache/sqoop/TestSqoopOptions.java
Lines 167 (patched)


Please remove System.out.println.


- Szabolcs Vasas


On Jan. 3, 2018, 11:16 a.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64715/
> ---
> 
> (Updated Jan. 3, 2018, 11:16 a.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/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/3/
> 
> 
> 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
> 
>

Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64715/#review194688
---




src/java/org/apache/sqoop/tool/ImportAllTablesTool.java
Lines 102 (patched)


I will move this inside the else block.


- Fero Szabo


On Jan. 3, 2018, 11:16 a.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64715/
> ---
> 
> (Updated Jan. 3, 2018, 11:16 a.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/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/3/
> 
> 
> 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
> 
>

Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64715/
---

(Updated Jan. 3, 2018, 11:16 a.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Changes
---

Here is a greatly changed diff as discussed with Szabi.
- it uses reflection to fill up a SqoopOptions object with random data
- the tests don't rely on any of the Tools anymore (the tools are responsible 
for creating a SqoopOptions objects in production, through their parseArguments 
methods)
- The two test cases are ensuring that the two separate conditions for clone 
are met: 
1. The cloned object is equal to the original
2. It's reference type fields are not the same (except for some fields that 
don't make sense to be cloned)

These tests ensure that, if somebody adds a new field to SqoopOptions, it has 
to be added to clone as well, otherwise the tests fail.

Concerns:
- please check exceptions from clone: do you agree with this list?
- I've removed a field from SqoopOptions, that wasn't used anywhere. Any 
concerns? 

Possible future work (refactor SqoopOptions entirely):
- SqoopOptions should be made immutable (though there is some application logic 
that seems to rely on it's mutability)
- It should be created by it's own factory or builder, not the Tools.
- A copy constructor should be used instead of clone()
- One could consider using a Map to store the many properties in SqoopOptions 
(instead of fields).


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

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


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

Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Fero Szabo via Review Board]

> On Dec. 22, 2017, 12:02 p.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/tool/ImportAllTablesTool.java
> > Line 109 (original), 110 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>

Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Fero Szabo via Review Board]

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

Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Fero Szabo via Review Board]

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


Changes
---

Hi Szabolcs,

Thanks, nice hint there, with commenting out... :) 

So I've created a test called testCloneCopiesAggregateFields to cover every 
part of clone. This initializes the SqoopOptions with valid values and then 
checks whether clone copies the fields.

I removed the testIsEqualToComparingFieldByFieldRecursivelyFail test, because I 
realized that testing for failure doesn't really make sense. A testcase like 
that doesn't really provide coverage for any change that introduces bugs.

The only remaining problem I see, is that if somebody adds a new field to 
SqoopOptions, it's not ensured to be cloned, and it's not tested if it's 
cloned. 
I will look into adding a new testcase that initializes each field with a radom 
value, based on this post: 
http://tuhrig.de/create-random-test-objects-with-java-reflection/


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/tool/ImportAllTablesTool.java d6d9f604 
  src/test/org/apache/sqoop/TestSqoopOptions.java 6d55c337 


Diff: https://reviews.apache.org/r/64715/diff/2/

Changes: https://reviews.apache.org/r/64715/diff/1-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

Re: Review Request 64715: SQOOP-3241 ImportAllTablesTool uses the same SqoopOptions object for every table import

[Szabolcs Vasas]

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


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)


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)


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