You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sqoop.apache.org by Abraham Fine <ab...@cloudera.com> on 2015/12/10 03:28:08 UTC

Review Request 41180: SQOOP-2727

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

Review request for Sqoop.


Bugs: SQOOP-2727
    https://issues.apache.org/jira/browse/SQOOP-2727


Repository: sqoop-sqoop2


Description
-------

Sqoop2: Use PreparedStatements for constructing GenericJDBCPartitions


Diffs
-----

  connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java ff33a4b 
  connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExtractor.java edb2754 
  connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java fa26c14 
  connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java 65400ef 
  connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartitioner.java 2a42ed4 
  connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java 3b52128 
  connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestPartitioner.java 3a767ab 

Diff: https://reviews.apache.org/r/41180/diff/


Testing
-------

yup


Thanks,

Abraham Fine

Re: Review Request 41180: SQOOP-2727

Posted by Jarek Cecho <ja...@apache.org>.


> On Dec. 10, 2015, 11:09 a.m., Jarek Cecho wrote:
> > Overall looks good to me. I have couple of comments inline. It seems that there is findbugs warning and test coverage decrease significantly - perhaps we should improve unit test coverage for the new code?

I've also rerun the precommit hook to see if the failure is relevant.


- Jarek


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


On Dec. 10, 2015, 2:28 a.m., Abraham Fine wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41180/
> -----------------------------------------------------------
> 
> (Updated Dec. 10, 2015, 2:28 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-2727
>     https://issues.apache.org/jira/browse/SQOOP-2727
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> Sqoop2: Use PreparedStatements for constructing GenericJDBCPartitions
> 
> 
> Diffs
> -----
> 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java ff33a4b 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExtractor.java edb2754 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java fa26c14 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java 65400ef 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartitioner.java 2a42ed4 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java 3b52128 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestPartitioner.java 3a767ab 
> 
> Diff: https://reviews.apache.org/r/41180/diff/
> 
> 
> Testing
> -------
> 
> yup
> 
> 
> Thanks,
> 
> Abraham Fine
> 
>

Re: Review Request 41180: SQOOP-2727

Posted by Jarek Cecho <ja...@apache.org>.

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


Overall looks good to me. I have couple of comments inline. It seems that there is findbugs warning and test coverage decrease significantly - perhaps we should improve unit test coverage for the new code?


connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java (lines 34 - 35)
<https://reviews.apache.org/r/41180/#comment169363>

    Good example of tests that we should add is the logic in readFields/writeWields - since we have now quite a lot of logic there what about writing a test cases for the serializatin/deserialization?



connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java (line 56)
<https://reviews.apache.org/r/41180/#comment169362>

    Shouldn't we add a space or something here when creating multi condition partition?



connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartitioner.java (lines 20 - 37)
<https://reviews.apache.org/r/41180/#comment169364>

    Seems as not relevant change? (just moving lines around that doesn't do anything)



connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestPartitioner.java (lines 20 - 35)
<https://reviews.apache.org/r/41180/#comment169365>

    Again seem as not relevant import-reordering?


Jarcec

- Jarek Cecho


On Dec. 10, 2015, 2:28 a.m., Abraham Fine wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41180/
> -----------------------------------------------------------
> 
> (Updated Dec. 10, 2015, 2:28 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-2727
>     https://issues.apache.org/jira/browse/SQOOP-2727
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> Sqoop2: Use PreparedStatements for constructing GenericJDBCPartitions
> 
> 
> Diffs
> -----
> 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java ff33a4b 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExtractor.java edb2754 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java fa26c14 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java 65400ef 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartitioner.java 2a42ed4 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java 3b52128 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestPartitioner.java 3a767ab 
> 
> Diff: https://reviews.apache.org/r/41180/diff/
> 
> 
> Testing
> -------
> 
> yup
> 
> 
> Thanks,
> 
> Abraham Fine
> 
>