> On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > Hi Venkat, > > thank you very much for incorporating all my suggestions. I've took a > > deeper look and the changes seems great to me. I do have couple of high > > level notes: > > > > 1) Tests for profile 200 and 23 are failing on obligate > > "java.lang.IncompatibleClassChangeError: Found interface > > org.apache.hadoop.mapreduce.JobContext, but class was expected". I was > > looking into maven central and it seems that only hadoop 1.x jars were > > published for HCatalog 0.11.0. Do you think that we can ask the > > HCatalog/Hive team to also publish Hadoop 2 compatible jars (via different > > classifier for example). > > > > 2) Would you mind updating user guide? > > > > 3) I'll update the old-pom.xml file after this will get in as I'm using it > > for bootstrapping my IntelliJ project and we're missing the new > > dependencies on HCatalog. > > > > I'm still missing running the patch on a real cluster, but otherwise I feel > > that we are very close to get it in! > >
Thanks for reviewing. 1) HIVE-4660 has been created to track that - I will check to see when it will be taken up - it is painful (and similar to the HBase situation). I built hcatalog locally with hadoop 2.x to test this with Hadoop. 2) I have a separate JIRA for that. Was waiting for the review of this. Will update the documentation patch with the doc update Thanks Venkat > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/ExportJobBase.java, line 467 > > <https://reviews.apache.org/r/10688/diff/6/?file=296857#file296857line467> > > > > Nit: Do you think that it would make sense to introduce a new FileType > > "HCATALOG"? Yes, good idea. Let me add that > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatExportFormat.java, lines > > 131-136 > > <https://reviews.apache.org/r/10688/diff/6/?file=296861#file296861line131> > > > > Is this method necessary? It seems to be only calling the parent method > > without any additional logic. I had a debug log message to track the record reader. Might have deleted by mistake. Will fix it. Thanks for the catch > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java, lines > > 243-260 > > <https://reviews.apache.org/r/10688/diff/6/?file=296866#file296866line243> > > > > The "home" variable seems to be unused after the assignment, so I'm > > assuming that assignements are not necessary? Yes. When I refactored the code, I missed this part. Let me fix it > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java, line 268 > > <https://reviews.apache.org/r/10688/diff/6/?file=296866#file296866line268> > > > > I do understand that Hive/HCatalog requires to have lowercase table > > names, but I'm a bit concerned about doing it without user knowledge. Do > > you think that it would make sense to detect if user specified uppercase > > letters and print out a warning? Sounds reasonable. If we detect that the table or database is not all in lowercase, we can issue a warning > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java, lines > > 975-976 > > <https://reviews.apache.org/r/10688/diff/6/?file=296866#file296866line975> > > > > Nit: This seems to be doing the same as > > SqoopOptions.getHCatHomeDefault(). Yes. Will change to use that > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/tool/BaseSqoopTool.java, line 1238 > > <https://reviews.apache.org/r/10688/diff/6/?file=296867#file296867line1238> > > > > It seems to me that the --as-avrodatafile and --as-sequencefile are > > also not compatible with the HCatalog import/export so we might add the > > validations here. Will add the validation > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/ExportJobBase.java, line 84 > > <https://reviews.apache.org/r/10688/diff/6/?file=296857#file296857line84> > > > > I can see the same variable isHCatJob in ImportJobBase and > > ExportJobBase. Do you think that it would make sense to put it into JobBase > > class instead? Yes, we can push it into JobBase. > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/manager/ConnManager.java, line 227 > > <https://reviews.apache.org/r/10688/diff/6/?file=296855#file296855line227> > > > > Nit: Can we throw here IllegalArgumentException similarly as in case of > > toAvroType() method? Dying fast seems to me as a better option that getting > > NPE somewhere later. I know that the toJavaType() is returning null as well > > at the moment, but we can "fix" it in follow up JIRA. Good point. Even though I handled it one place (with explicit exception, it may be better to through this early). > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/manager/ConnManager.java, line 223 > > <https://reviews.apache.org/r/10688/diff/6/?file=296855#file296855line223> > > > > Nit: The indent seems to be off. Will fix > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/config/ConfigurationConstants.java, line 96 > > <https://reviews.apache.org/r/10688/diff/6/?file=296853#file296853line96> > > > > Introducing this property is awesome idea, I would suggest to also use > > it across entire code base (for example in JobBase class). Considering the > > size of this patch already, I'm completely fine with doing that in follow > > up JIRA. Yes, we should use fix other uses of this string. We can address it in a follow-on JIRA > On May 28, 2013, 9:33 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/SqoopOptions.java, line 1313 > > <https://reviews.apache.org/r/10688/diff/6/?file=296852#file296852line1313> > > > > Nit: It seems that the SqoopOptions class is preferring get methods > > returning boolean to be called with either "is" or "do", e.g. something > > like isCreateHCatalogTable(). Sure. will change - Venkat ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10688/#review21079 ----------------------------------------------------------- On May 24, 2013, 11:18 p.m., Venkat Ranganathan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10688/ > ----------------------------------------------------------- > > (Updated May 24, 2013, 11:18 p.m.) > > > 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 > ----- > > build.xml 636c103 > ivy.xml 1fa4dd1 > ivy/ivysettings.xml c4cc561 > 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/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/HCatalogTestUtils.java PRE-CREATION > src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java PRE-CREATION > src/test/org/apache/sqoop/hcat/TestHCatalogExport.java PRE-CREATION > src/test/org/apache/sqoop/hcat/TestHCatalogImport.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 > >
