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