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