sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anna Szonyi <szo...@cloudera.com>
Subject Re: Review Request 51912: SQOOP 816 - Scoop and support for external Hive tables
Date Mon, 24 Apr 2017 15:37:58 GMT

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



Hi Chris,

Thank you for yout contribution, thisis a great change! There are a few places I found that
can -and probably will- cause NullPointerExceptions, please fix these, as they're currently
causing some tests to fail as well.
It might be out of scope for this review, but it would be great if you could add new tests
for to the TestHiveImport test class which can cover an external hive table import end to
end (it could even be a parameterized test, so all tests would run once for external and once
for non-external :)). 

Otherwise the change looks pretty nice and clear, thanks! :)

Thanks,
Anna


src/java/org/apache/sqoop/hive/TableDefWriter.java
Lines 136 (patched)
<https://reviews.apache.org/r/51912/#comment245834>

    This line has a potential NPE in it (currently some tests run with ant test fail with
NPE on one of these lines).
    Replacing it with StringUtils.isBlank() would help with this.
    It might also make sense to extract the whole condition like isHiveExternalTableSet or
something similar



src/java/org/apache/sqoop/hive/TableDefWriter.java
Lines 142 (patched)
<https://reviews.apache.org/r/51912/#comment245835>

    This line has a potential NPE.
    Replacing it with StringUtils.isBlank()  would help with this.
    It might also make sense to extract the whole condition like isHiveExternalTableSet or
something similar.



src/java/org/apache/sqoop/hive/TableDefWriter.java
Lines 231 (patched)
<https://reviews.apache.org/r/51912/#comment245836>

    This line has a potential NPE in it.
    Replacing it with StringUtils.isBlank()  would help with this.
    It might also make sense to extract the whole condition like isHiveExternalTableSet or
something similar



src/java/org/apache/sqoop/tool/BaseSqoopTool.java
Lines 1583 (patched)
<https://reviews.apache.org/r/51912/#comment245833>

    This line has a potential NPE.
    Replacing it with StringUtils.isBlank()  would help with this.
    It might also make sense to extract the whole condition to make it a bit more easy to
understand, e.g. isNotHiveImportButExternalTableDirIsSet or something similar :).



src/test/org/apache/sqoop/hive/TestTableDefWriter.java
Lines 82 (patched)
<https://reviews.apache.org/r/51912/#comment245842>

    Thanks for creating a new test class! Would you please also add test cases that cover
the "default path", where we're not creating external tables, to ensure that is still working
as expected?


- Anna Szonyi


On April 21, 2017, 1:45 p.m., Chris Teoh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51912/
> -----------------------------------------------------------
> 
> (Updated April 21, 2017, 1:45 p.m.)
> 
> 
> Review request for Sqoop, Kathleen Ting, Attila Szabo, and Liz Szilagyi.
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> SQOOP-816 - Hive External table support. Added --as-external-table option and --table-location
option
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/SqoopOptions.java 801942e 
>   src/java/org/apache/sqoop/hive/TableDefWriter.java 32fcca3 
>   src/java/org/apache/sqoop/tool/BaseSqoopTool.java 46f405f 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java PRE-CREATION 
>   src/test/org/apache/sqoop/tool/TestImportTool.java 7e11f54 
> 
> 
> Diff: https://reviews.apache.org/r/51912/diff/5/
> 
> 
> Testing
> -------
> 
> Unit tests.
> Interactive IDE debug step throughs.
> No testing on HiveServer as SQOOP-2787 may not yet be in trunk.
> End to end testing on PostgresQL to HiveServer performed.
> 
> 
> Thanks,
> 
> Chris Teoh
> 
>


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