You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Shruti Joshi <sh...@persistent.co.in> on 2013/06/03 15:04:24 UTC

Re: Review Request: Fix for SQOOP-107

-----------------------------------------------------------
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 (updated)
-----

  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

Re: Review Request: Fix for SQOOP-107

Posted by Shruti Joshi <sh...@persistent.co.in>.

> On June 8, 2013, 4:19 p.m., Jarek Cecho wrote:
> > 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.

Hi Jarek,
Thanks a lot for your suggestions.
I tried to run HBase tests by specifying -Dhadoopversion=100 and also keeping the same IP for localhost and machine hostname in /etc/hosts.
But still I am facing the same issues in my environment and hence can you please try to run this test in your environment?
I am uploading the patch again with all the required changes and suggestions incorporated.

Shruti


> On June 8, 2013, 4:19 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java, lines 180-182
> > <https://reviews.apache.org/r/11041/diff/4/?file=299930#file299930line180>
> >
> >     What about keeping the current behavior and allow user to decide whether to include the columns used for row key in the data themselves?

I have modified code so as to keep the current behavior as it is Viz. including row key columns in the data as per the user’s choice


> On June 8, 2013, 4:19 p.m., Jarek Cecho wrote:
> > src/test/com/cloudera/sqoop/hbase/HBaseImportTest.java, lines 158-159
> > <https://reviews.apache.org/r/11041/diff/4/?file=299931#file299931line158>
> >
> >     Please note that the Sqoop parameters must be appended to the end of the array not to it's beginning.

Thanks for the suggestion.
I have followed the conventions used in the existing HBase tests while writing this test


- Shruti


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11041/#review21580
-----------------------------------------------------------


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
> 
>

Re: Review Request: Fix for SQOOP-107

Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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
> 
>

Re: Review Request: Fix for SQOOP-107

Posted by Shruti Joshi <sh...@persistent.co.in>.

> On June 19, 2013, 8:50 p.m., Jarek Cecho wrote:
> > Hi Shruti,
> > the patch no longer applies on the HEAD of branch trunk. Would you mind rebasing it? I think that the culprit here is SQOOP-1083, where we split the HBase tests into multiple files, so the change should be rather straightforward.

I have rebased my code and added composite key test-case in src/test/com/cloudera/sqoop/hbase/HBaseImportAddRowKeyTest.java


> On June 19, 2013, 8:50 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java, lines 96-97
> > <https://reviews.apache.org/r/11041/diff/5/?file=308349#file308349line96>
> >
> >     Nit: Assuming that we are only iterating over the generated array, is there a reason why we need the compositeKeyAttributes defined as List? Shouldn't the array returned by the split() method be sufficient? I'm not quite sure what is the gain here to transform the Array into List.

The compositeKeyAttributes is required to be of type List, because in 'putRecordInHBase' method, we have to check whether the column is a part of composite key or not using 'contains' method.
If it is of type Array, we need to iterate using 'for loop'.


- Shruti


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11041/#review22119
-----------------------------------------------------------


On June 20, 2013, 8:49 a.m., Shruti Joshi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11041/
> -----------------------------------------------------------
> 
> (Updated June 20, 2013, 8:49 a.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/HBaseImportAddRowKeyTest.java bd1cd2d 
> 
> Diff: https://reviews.apache.org/r/11041/diff/
> 
> 
> Testing
> -------
> 
> ant test (default) was a success.
> 
> 
> Thanks,
> 
> Shruti Joshi
> 
>

Re: Review Request: Fix for SQOOP-107

Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11041/#review22119
-----------------------------------------------------------


Hi Shruti,
the patch no longer applies on the HEAD of branch trunk. Would you mind rebasing it? I think that the culprit here is SQOOP-1083, where we split the HBase tests into multiple files, so the change should be rather straightforward.


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

    Nit: Assuming that we are only iterating over the generated array, is there a reason why we need the compositeKeyAttributes defined as List? Shouldn't the array returned by the split() method be sufficient? I'm not quite sure what is the gain here to transform the Array into List.



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

    The patch is calling getArvg method with parameter includeHadoopFlags set to true. As a result the returned array will contain couple -D properties defined at the beginning of the array and rest of the Sqoop parameters after that. As Sqoop requires to have all -D parameters prior all normal arguments putting --hbase-row-key at the begging of the array won't work and the test itself is failing. I've done following code change that make the test passing:
    
        argv[0]="-D";
        argv[1]="sqoop.hbase.add.row.key=true";
        System.arraycopy(otherArg, 0, argv, 2, otherArg.length);
        argv[argv.length - 2] = "--hbase-row-key";
        argv[argv.length - 1] = getColName(0)+","+getColName(1);


Jarcec

- Jarek Cecho


On June 19, 2013, 10:17 a.m., Shruti Joshi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11041/
> -----------------------------------------------------------
> 
> (Updated June 19, 2013, 10:17 a.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
> 
>

Re: Review Request: Fix for SQOOP-107

Posted by Shruti Joshi <sh...@persistent.co.in>.

> On June 20, 2013, 4:20 p.m., Jarek Cecho wrote:
> > Please upload the patch to the JIRA and I'll commit it.

Hi Jarek,
Thanks a lot.
I have uploaded the patch to the JIRA.

Shruti


- Shruti


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11041/#review22165
-----------------------------------------------------------


On June 20, 2013, 8:49 a.m., Shruti Joshi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11041/
> -----------------------------------------------------------
> 
> (Updated June 20, 2013, 8:49 a.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/HBaseImportAddRowKeyTest.java bd1cd2d 
> 
> Diff: https://reviews.apache.org/r/11041/diff/
> 
> 
> Testing
> -------
> 
> ant test (default) was a success.
> 
> 
> Thanks,
> 
> Shruti Joshi
> 
>

Re: Review Request: Fix for SQOOP-107

Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11041/#review22165
-----------------------------------------------------------

Ship it!


Please upload the patch to the JIRA and I'll commit it.

- Jarek Cecho


On June 20, 2013, 8:49 a.m., Shruti Joshi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11041/
> -----------------------------------------------------------
> 
> (Updated June 20, 2013, 8:49 a.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/HBaseImportAddRowKeyTest.java bd1cd2d 
> 
> Diff: https://reviews.apache.org/r/11041/diff/
> 
> 
> Testing
> -------
> 
> ant test (default) was a success.
> 
> 
> Thanks,
> 
> Shruti Joshi
> 
>

Re: Review Request: Fix for SQOOP-107

Posted by Shruti Joshi <sh...@persistent.co.in>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11041/
-----------------------------------------------------------

(Updated June 20, 2013, 8:49 a.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 (updated)
-----

  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/HBaseImportAddRowKeyTest.java bd1cd2d 

Diff: https://reviews.apache.org/r/11041/diff/


Testing
-------

ant test (default) was a success.


Thanks,

Shruti Joshi

Re: Review Request: Fix for SQOOP-107

Posted by Shruti Joshi <sh...@persistent.co.in>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11041/
-----------------------------------------------------------

(Updated June 19, 2013, 10:17 a.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 (updated)
-----

  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