----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66300/#review200113 -----------------------------------------------------------
Hi Dani, Thank you for starting this initiative! The changes look good to me, ran the unit and third party tests successfully, I have left some minor comments only. I guess we do not want to commit this patch until the final version of Hive 3 is released, am I right? Do you know when it is going to happen? ivy.xml Lines 95 (patched) <https://reviews.apache.org/r/66300/#comment280760> Can we move the version to libraries.properties? src/java/org/apache/sqoop/hive/HiveImport.java Lines 368 (patched) <https://reviews.apache.org/r/66300/#comment280761> Is it possible that before setting DerbyPolicy another Policy was set? In that case I think we should restore the original Policy. src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java Lines 69 (patched) <https://reviews.apache.org/r/66300/#comment280767> Can we use List interface and diamond operator here? src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java Lines 71 (patched) <https://reviews.apache.org/r/66300/#comment280762> Missing @Override annotation. src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java Lines 75 (patched) <https://reviews.apache.org/r/66300/#comment280763> Missing @Override annotation. src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java Lines 76 (patched) <https://reviews.apache.org/r/66300/#comment280766> Can we use for-each loop here? src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java Lines 84 (patched) <https://reviews.apache.org/r/66300/#comment280764> Missing @Override annotation. src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java Lines 88 (patched) <https://reviews.apache.org/r/66300/#comment280765> Missing @Override annotation. src/java/org/apache/sqoop/util/SqoopJsonUtil.java Line 42 (original), 42 (patched) <https://reviews.apache.org/r/66300/#comment280769> Casing: getJsonStringForMap src/java/org/apache/sqoop/util/SqoopJsonUtil.java Lines 44 (patched) <https://reviews.apache.org/r/66300/#comment280768> It is a matter of taste, I am not a big fan of changing the parameter, maybe we could change it to something like this: if (map == null) { return EMPTY_JSON_MAP; } where EMPTY_JSON_MAP would be a constant extracted from isEmptyJSON method ("{}") - Szabolcs Vasas On March 27, 2018, 8:50 a.m., daniel voros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66300/ > ----------------------------------------------------------- > > (Updated March 27, 2018, 8:50 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3305 > https://issues.apache.org/jira/browse/SQOOP-3305 > > > Repository: sqoop-trunk > > > Description > ------- > > To be able to eventually support the latest versions of Hive, HBase and > Accumulo, we should start by upgrading our Hadoop dependencies to 3.0.0. See > https://hadoop.apache.org/docs/r3.0.0/index.html > > > Diffs > ----- > > ivy.xml 6be4fa2 > ivy/libraries.properties c44b50b > src/java/org/apache/sqoop/config/ConfigurationHelper.java e07a699 > src/java/org/apache/sqoop/hive/HiveImport.java c272911 > src/java/org/apache/sqoop/mapreduce/JobBase.java 6d1e049 > src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java 784b5f2 > src/java/org/apache/sqoop/util/SqoopJsonUtil.java adf186b > src/test/org/apache/sqoop/TestSqoopOptions.java bb7c20d > testdata/hcatalog/conf/hive-site.xml edac7aa > > > Diff: https://reviews.apache.org/r/66300/diff/1/ > > > Testing > ------- > > Normal and third-party unit tests. > > > Thanks, > > daniel voros > >
