sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request: Fix for SQOOP-107
Date Sat, 08 Jun 2013 16:19:20 GMT

-----------------------------------------------------------
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.)
> 
> 
> Review request for Sqoop.
> 
> 
> Description
> -------
> 
> Sqoop does not support importing data into hbase when table contains composite key.
> 
> 
> This addresses bug https://issues.cloudera.org/browse/SQOOP-107.
>     https://issues.apache.org/jira/browse/https://issues.cloudera.org/browse/SQOOP-107
> 
> 
> Diffs
> -----
> 
>   src/docs/man/hbase-args.txt 7164e93 
>   src/docs/user/hbase-args.txt 36b930b 
>   src/docs/user/hbase.txt 24c8df8 
>   src/java/org/apache/sqoop/hbase/HBasePutProcessor.java 6aca97f 
>   src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java 13c765c 
>   src/test/com/cloudera/sqoop/hbase/HBaseImportTest.java d411f3d 
> 
> Diff: https://reviews.apache.org/r/11041/diff/
> 
> 
> Testing
> -------
> 
> ant test (default) was a success.
> 
> 
> Thanks,
> 
> Shruti Joshi
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message