sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Attila Szabo <mau...@apache.org>
Subject Re: Review Request 58600: add --schema option for import-all-tables and list-tables against db2
Date Wed, 17 May 2017 11:50:02 GMT

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


Fix it, then Ship it!




Hey Angela,

Many thanks for providing this contribution.
In general your changeset looks good, I've just found some codestyle issues, JDBC connection
handling, and logging conventions.

Could you please check them, and apply the changes / answer where I've raised questions?

After we have the amendments I will be able to commit your changes!

Many thanks,
Attila


src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 90-94 (patched)
<https://reviews.apache.org/r/58600/#comment248686>

    Please fix indentation + formatting here!



src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 181 (patched)
<https://reviews.apache.org/r/58600/#comment248687>

    Is this commit call neded here?
    We've just only executed SELECT statement. What kind of changes we'd like to commit?



src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 184 (patched)
<https://reviews.apache.org/r/58600/#comment248688>

    I have the same concerns as around commit. Could you please give some more insights around
why is this call needed? (FYI: I'm not a DB2 expert, so it's very possible I'm just not aware
about the internals)



src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 229 (patched)
<https://reviews.apache.org/r/58600/#comment248674>

    Please remove the trailing whitesapce!



src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 246-249 (patched)
<https://reviews.apache.org/r/58600/#comment248689>

    Same concerns with commit+rollback as above



src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 372-394 (patched)
<https://reviews.apache.org/r/58600/#comment248685>

    Could you please check if this has been already solved in other connectors?
    
    Getting schema option sounds quite general, might have been implemented elsewhere in the
codebase.
    
    Would make sense to abstract it out, and just call it from different places.
    
    Thanks for checking!



src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 382 (patched)
<https://reviews.apache.org/r/58600/#comment248675>

    Please remove trailing whitesapces!



src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 393 (patched)
<https://reviews.apache.org/r/58600/#comment248676>

    Please remove trailing whitesapces!



src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 153 (patched)
<https://reviews.apache.org/r/58600/#comment248677>

    Please remove trailing whitesapces!



src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 166 (patched)
<https://reviews.apache.org/r/58600/#comment248683>

    Please include debug level stack trace logging here!



src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 233 (patched)
<https://reviews.apache.org/r/58600/#comment248682>

    Please include debug level stack trace logging!



src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 245 (patched)
<https://reviews.apache.org/r/58600/#comment248681>

    Please include debug level stacktrace loggging here as well!



src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 258 (patched)
<https://reviews.apache.org/r/58600/#comment248680>

    How is this change related to the oraoop module?
    AFAIK DB2 execution path has to be independent from the oraoop execution path.
    
    What kind of problem did you face which led you to include this config settings?



src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 264 (patched)
<https://reviews.apache.org/r/58600/#comment248679>

    Please do remove printStackTrace calls.
    Logging the exception is the conventional way in the Sqoop codebase.
    If you'd like to add the stacktrace to the log as well (which is a best practice and very
much advised), please add another line like:
    LOG.debug("Sqoop exception details", e);



src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 270 (patched)
<https://reviews.apache.org/r/58600/#comment248678>

    Please fix indendtation according to Sqoop guideline (here two leading spaces are missing)


- Attila Szabo


On April 21, 2017, 2:44 a.m., Ying Cao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
> 
> (Updated April 21, 2017, 2:44 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo,
and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-1905
>     https://issues.apache.org/jira/browse/SQOOP-1905
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef 
>   src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java PRE-CREATION

> 
> 
> Diff: https://reviews.apache.org/r/58600/diff/1/
> 
> 
> Testing
> -------
> 
> Mannual UT  is passed
> 
> 
> Thanks,
> 
> Ying Cao
> 
>


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