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

Switch to Plain View
Sqoop >> mail # dev >> Review Request: Fix for SQOOP-1029


+
Venkat Ranganathan 2013-05-04, 23:54
+
Venkat Ranganathan 2013-05-06, 05:04
+
Jarek Cecho 2013-05-20, 13:15
+
Jarek Cecho 2013-05-20, 13:15
+
Jarek Cecho 2013-05-06, 04:32
+
Venkat Ranganathan 2013-05-06, 05:04
+
Shruti Joshi 2013-05-10, 12:06
+
Jarek Cecho 2013-05-21, 08:56
+
Shruti Joshi 2013-05-23, 07:55
+
Shruti Joshi 2013-05-23, 07:52
+
Shruti Joshi 2013-05-22, 13:22
+
Shruti Joshi 2013-05-27, 11:22
+
Jarek Cecho 2013-05-28, 10:24
+
Jarek Cecho 2013-05-29, 06:54
+
Shruti Joshi 2013-05-28, 12:16
+
Shruti Joshi 2013-05-30, 13:12
+
Shruti Joshi 2013-06-03, 13:06
+
Shruti Joshi 2013-06-03, 13:04
Copy link to this message
-
Re: Review Request: Fix for SQOOP-107

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11041/#review21580
-----------------------------------------------------------
Hi Shruti,
thank you very much for incorporating all my suggestions. I've dived into the patch and I do have couple of notes.

Failing tests might have a lot of causes and it's hard to guess without entire log. The HBase tests are particular hard to debug as they contain a lot of information and it's sometimes hard to get oriented as the cause can be hidden somewhere in the middle of the log and not necessary at the end. Some of known hiccups with HBase tests:

1) If you are running on ubuntu, then it will by default have /etc/hosts file with two entries - "127.0.0.1 localhost" and "127.0.1.1 $hostname". This won't work for HBase MiniCluster as both localhost and machine hostname must point to the same IP.

2) HBase team is not yet releasing artifacts compatible with Hadoop 2.0, so HBase related tests are working only in profiles "20" and "100". Please note that default profile is "23" (thus Hadoop 2) and therefore without specifying -Dhadoopversion=100, HBase tests are expected to fail.
src/docs/user/hbase-args.txt
<https://reviews.apache.org/r/11041/#comment44622>

    Nit: Please remove trailing white space characters.

src/java/org/apache/sqoop/hbase/HBasePutProcessor.java
<https://reviews.apache.org/r/11041/#comment44623>

    Nit: Can we change name of this method to suggest what it's doing, maybe "detectCompositeKey()"?

src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java
<https://reviews.apache.org/r/11041/#comment44624>

    Nit: I would suggest to tweak the exception to be more descriptive, for example "Row key column can't be NULL"?
    Nit: I don't think that it's necessary to log and thrown the same text, maybe just throwing an exception will be sufficient?

src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java
<https://reviews.apache.org/r/11041/#comment44625>

    Nit: I would suggest to reword the exception a bit, for example "Column family can't be NULL".
    Nit: I don't think that it's necessary to log and thrown the same text, maybe just throwing an exception will be sufficient?

src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java
<https://reviews.apache.org/r/11041/#comment44626>

    I would suggest to move operations that can be done only once to a context that will be executed only once rather than repeating the same operations for every row.

src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java
<https://reviews.apache.org/r/11041/#comment44627>

    Nit: I don't think that it's necessary to log and thrown the same text, maybe just throwing an exception will be sufficient?
    

src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java
<https://reviews.apache.org/r/11041/#comment44629>

    What about using StringUtils.join(DELIMITER_HBASE, rowKeyList) instead of home brew method doing essentially the same?

src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java
<https://reviews.apache.org/r/11041/#comment44628>

    Nit: The indent seems to be off.

src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java
<https://reviews.apache.org/r/11041/#comment44630>

    What about keeping the current behavior and allow user to decide whether to include the columns used for row key in the data themselves?

src/test/com/cloudera/sqoop/hbase/HBaseImportTest.java
<https://reviews.apache.org/r/11041/#comment44643>

    Please note that the Sqoop parameters must be appended to the end of the array not to it's beginning.
Jarcec

- Jarek Cecho
On June 3, 2013, 1:04 p.m., Shruti Joshi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11041/
> -----------------------------------------------------------
>
> (Updated June 3, 2013, 1:04 p.m.)
+
Shruti Joshi 2013-06-19, 10:12
+
Shruti Joshi 2013-06-19, 10:17
+
Shruti Joshi 2013-06-20, 08:50
+
Jarek Cecho 2013-06-20, 16:20
+
Shruti Joshi 2013-06-21, 04:12
+
Jarek Cecho 2013-06-19, 20:50
+
Shruti Joshi 2013-06-21, 03:40
+
Venkat Ranganathan 2013-05-21, 01:00
+
Venkat Ranganathan 2013-05-21, 04:09
+
Jarek Cecho 2013-05-21, 08:39
+
Venkat Ranganathan 2013-05-21, 16:42
+
Venkat Ranganathan 2013-05-21, 18:57
+
Jarek Cecho 2013-05-22, 06:31
+
Venkat Ranganathan 2013-05-30, 06:07
+
Venkat Ranganathan 2013-05-30, 18:25
+
Abraham Elmahrek 2013-05-31, 20:30
+
Venkat Ranganathan 2013-06-01, 00:07
+
Abraham Elmahrek 2013-05-31, 20:33
+
Jarek Cecho 2013-06-10, 15:36
+
Venkat Ranganathan 2013-06-10, 16:06
+
Venkat Ranganathan 2013-06-11, 18:41
+
Venkat Ranganathan 2013-06-12, 17:23
+
Venkat Ranganathan 2013-06-13, 05:46
+
Abraham Elmahrek 2013-06-13, 19:00
+
Venkat Ranganathan 2013-06-13, 20:32
+
Jarek Cecho 2013-06-13, 20:35
+
Abraham Elmahrek 2013-06-13, 01:41
+
Venkat Ranganathan 2013-06-13, 05:44
+
Venkat Ranganathan 2013-06-19, 21:48
+
Jarek Cecho 2013-06-20, 22:02