> On June 6, 2013, 6:34 p.m., Jarek Cecho wrote:
> > Hi Venkat,
> > thank you very much for incorporating all my suggestions. I believe that we
> > are almost at the end. I was again doing some testing and I've noticed few
> > issues (some of them created by my own suggestions):
> >
> > 1) I see compilation failure
> > [javac]
> > /home/jarcec/apache/repos/sqoop/src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java:877:
> > join(java.lang.CharSequence,java.lang.Iterable<?>) in
> > org.apache.hadoop.util.StringUtils cannot be applied to
> > (java.lang.String,java.lang.String[])
> > [javac] String argLine = StringUtils.join(",", argArray);
> >
> > I've fixed that by changing the line to String argLine =
> > StringUtils.join(",", Arrays.asList(argArray)) to unblock the review,
> > however proper solution is up to you :-)
> >
> > 2) We've changed the hardcoded paths to Hive and HCatalog home to
> > SqoopOptions.getHiveHomeDefault() (or HCatalog), however those two methods
> > actually can return null, which is causing ClassNotFoundExceptions later in
> > the code. What about improving them in similar fashion:
> >
> > public static String getHiveHomeDefault() {
> > // Set this with $HIVE_HOME, but -Dhive.home can override.
> > String hiveHome = System.getenv("HIVE_HOME", "/usr/lib/hive");
> > return System.getProperty("hive.home", hiveHome);
> > }
Thanks for the review
1) I did run all the tests with hadoop100 profile but it looks like
StringUtils.join(String, String[]) is a new addition. Unfortunately, there is
no @since in the javadocs :( Sorry about that
2) Good catch - will fix it and use the default values I was using before for
these two
> On June 6, 2013, 6:34 p.m., Jarek Cecho wrote:
> > bin/configure-sqoop, line 118
> > <https://reviews.apache.org/r/10688/diff/10/?file=301227#file301227line118>
> >
> > Nit: Add HCatalog to dependency list
Will fix
> On June 6, 2013, 6:34 p.m., Jarek Cecho wrote:
> > bin/configure-sqoop, line 118
> > <https://reviews.apache.org/r/10688/diff/10/?file=301227#file301227line118>
> >
> > Nit: Add HCatalog to dependency list
Will fix
> On June 6, 2013, 6:34 p.m., Jarek Cecho wrote:
> > bin/configure-sqoop, line 120
> > <https://reviews.apache.org/r/10688/diff/10/?file=301227#file301227line120>
> >
> > Rest of the Sqoop is expecting variable HADOOP_COMMON_HOME whereas the
> > underlying hcat script is expecting HADOOP_HOME, so on BigTop this line is
> > ending with:
> >
> > Hadoop not found.
> >
> > I was able to workaround it by adding following line before the
> > highlighted line:
> >
> > export HADOOP_HOME=$HADOOP_COMMON_HOME
> >
> > However I'm not sure whether this is the best solution or not :-/
I think that sounds like a good fix. Thanks for that. Let me add it and also
add a comment so that it is not accidentally removed in future
- Venkat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10688/#review21527
-----------------------------------------------------------
On June 6, 2013, midnight, Venkat Ranganathan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10688/
> -----------------------------------------------------------
>
> (Updated June 6, 2013, midnight)
>
>
> Review request for Sqoop and Jarek Cecho.
>
>
> Description
> -------
>
> This patch implements the new feature of integrating HCatalog and Sqoop.
> With this feature, it is possible to import and export data between Sqoop and
> HCatalog tables. The document attached to SQOOP-931 JIRA issue discusses
> the high level appraches.
>
> With this integration, more fidelity can be brought to the process of moving
> data between enterprise data stores and hadoop ecosystem.
>
>
> Diffs
> -----
>
> bin/configure-sqoop 61ff3f2
> build.xml 636c103
> ivy.xml 1fa4dd1
> ivy/ivysettings.xml c4cc561
> src/docs/user/SqoopUserGuide.txt 01ac1cf
> src/docs/user/hcatalog.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java f18d43e
> src/java/org/apache/sqoop/config/ConfigurationConstants.java 5354063
> src/java/org/apache/sqoop/hive/HiveImport.java 838f083
> src/java/org/apache/sqoop/manager/ConnManager.java a1ac38e
> src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java ef1d363
> src/java/org/apache/sqoop/mapreduce/ExportJobBase.java 1065d0b
> src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 2465f3f
> src/java/org/apache/sqoop/mapreduce/JdbcExportJob.java 20636a0
> src/java/org/apache/sqoop/mapreduce/JobBase.java 0df1156
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatExportFormat.java
> PRE-CREATION
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatExportMapper.java
> PRE-CREATION
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportMapper.java
> PRE-CREATION
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatInputSplit.java
> PRE-CREATION
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatRecordReader.java
> PRE-CREATION
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java
> PRE-CREATION
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 42f521f
> src/java/org/apache/sqoop/tool/CodeGenTool.java dd34a97
> src/java/org/apache/sqoop/tool/ExportTool.java 215addd
> src/java/org/apache/sqoop/tool/ImportTool.java 2627726
> src/perftest/ExportStressTest.java 0a41408
> src/test/com/cloudera/sqoop/ThirdPartyTests.java 06f7122
> src/test/com/cloudera/sqoop/hive/TestHiveImport.java 462ccf1
> src/test/com/cloudera/sqoop/testutil/BaseSqoopTestCase.java cf41b96
> src/test/com/cloudera/sqoop/testutil/ExportJobTestCase.java e13f3df
> src/test/org/apache/sqoop/hcat/HCatalogExportTest.java PRE-CREATION
> src/test/org/apache/sqoop/hcat/HCatalogImportTest.java PRE-CREATION
> src/test/org/apache/sqoop/hcat/HCatalogTestUtils.java PRE-CREATION
> src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java PRE-CREATION
> testdata/hcatalog/conf/hive-log4j.properties PRE-CREATION
> testdata/hcatalog/conf/hive-site.xml PRE-CREATION
> testdata/hcatalog/conf/log4j.properties PRE-CREATION
>
> Diff: https://reviews.apache.org/r/10688/diff/
>
>
> Testing
> -------
>
> Two new integration test suites with more than 20 tests in total have been
> added to test various aspects of the integration. A unit test to test the
> option management is also added. All tests pass
>
>
> Thanks,
>
> Venkat Ranganathan
>
>
