Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Sqoop, mail # dev - Review Request: SQOOP-931 - Integration of Sqoop and HCatalog


+
Venkat Ranganathan 2013-04-21, 05:51
+
Venkat Ranganathan 2013-04-24, 05:13
+
Venkat Ranganathan 2013-04-29, 23:21
+
Venkat Ranganathan 2013-04-30, 06:56
+
Venkat Ranganathan 2013-05-04, 23:46
+
Jarek Cecho 2013-05-20, 13:02
+
Venkat Ranganathan 2013-05-21, 00:35
+
Jarek Cecho 2013-05-21, 10:09
+
Venkat Ranganathan 2013-05-24, 23:18
+
Jarek Cecho 2013-05-28, 09:33
Copy link to this message
-
Re: Review Request: SQOOP-931 - Integration of Sqoop and HCatalog
Venkat Ranganathan 2013-05-28, 20:38


> 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

Yes, we can push it into JobBase.

Good point.  Even though I handled it one place (with explicit exception, it may be better to through this early).

Will fix

Yes, we should use fix other uses of this string.  We can address it in a follow-on JIRA

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:
+
Venkat Ranganathan 2013-05-29, 20:55
+
Venkat Ranganathan 2013-06-02, 20:33
+
Venkat Ranganathan 2013-06-03, 04:16
+
Jarek Cecho 2013-06-04, 23:15
+
Venkat Ranganathan 2013-06-05, 00:09
+
Venkat Ranganathan 2013-06-05, 03:52
+
Venkat Ranganathan 2013-06-05, 21:42
+
Jarek Cecho 2013-06-05, 21:26
+
Venkat Ranganathan 2013-06-06, 00:00
+
Venkat Ranganathan 2013-06-06, 22:55
+
Venkat Ranganathan 2013-06-07, 02:03
+
Jarek Cecho 2013-06-07, 14:29
+
Jarek Cecho 2013-06-07, 01:03
+
Venkat Ranganathan 2013-06-07, 01:53
+
Jarek Cecho 2013-06-06, 18:34
+
Venkat Ranganathan 2013-06-06, 19:07