sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Teoh <chris.t...@gmail.com>
Subject Re: Review Request 51912: SQOOP 816 - Scoop and support for external Hive tables
Date Wed, 26 Apr 2017 12:53:53 GMT


> On April 24, 2017, 3:37 p.m., Anna Szonyi wrote:
> > 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

Thanks Anna. I did try to write an integration test but I was getting a stuck as the dependencies
used in the build/test seem to be incompatible with my virtual machine, exceptions about protobuf
callId missing when trying to interact with HDFS on my virtual machine. This was why I provided
the manual test instructions.


> On April 24, 2017, 3:37 p.m., Anna Szonyi wrote:
> > src/java/org/apache/sqoop/hive/TableDefWriter.java
> > Lines 136 (patched)
> > <https://reviews.apache.org/r/51912/diff/5/?file=1692717#file1692717line136>
> >
> >     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

Thanks Anna. Patch updated.


> On April 24, 2017, 3:37 p.m., Anna Szonyi wrote:
> > src/java/org/apache/sqoop/hive/TableDefWriter.java
> > Lines 142 (patched)
> > <https://reviews.apache.org/r/51912/diff/5/?file=1692717#file1692717line142>
> >
> >     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.

Thanks Anna. Patch updated.


> On April 24, 2017, 3:37 p.m., Anna Szonyi wrote:
> > src/java/org/apache/sqoop/hive/TableDefWriter.java
> > Lines 231 (patched)
> > <https://reviews.apache.org/r/51912/diff/5/?file=1692717#file1692717line231>
> >
> >     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

Thanks Anna. Patch updated.


> On April 24, 2017, 3:37 p.m., Anna Szonyi wrote:
> > src/java/org/apache/sqoop/tool/BaseSqoopTool.java
> > Lines 1583 (patched)
> > <https://reviews.apache.org/r/51912/diff/5/?file=1692718#file1692718line1583>
> >
> >     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 :).

Thanks Anna. Patch updated.


> On April 24, 2017, 3:37 p.m., Anna Szonyi wrote:
> > src/test/org/apache/sqoop/hive/TestTableDefWriter.java
> > Lines 82 (patched)
> > <https://reviews.apache.org/r/51912/diff/5/?file=1692719#file1692719line82>
> >
> >     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?

Thanks Anna. Patch updated.


- Chris


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


On April 26, 2017, 12:52 p.m., Chris Teoh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51912/
> -----------------------------------------------------------
> 
> (Updated April 26, 2017, 12:52 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/6/
> 
> 
> 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