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 Tue, 21 May 2013 08:56:55 GMT

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


Hi Shruti,
thank you very much for working on this, greatly appreciated. I do have couple of high level
notes:

1) Would you mind update the documentation describing the new functionality? You can find
the docs in src/docs/ directory.

2) Would you mind adding a tests that will exercise the added functionality?

3) Please run "ant checkstyle" to check out the coding style.


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

    Nit: Is this change necessary?



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

    Nit: Is this change necessary?



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

    What about using names suggesting the usage rather than content of the variable? Perhaps
DELIMITER_COMMAND_LINE?



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

    What about using names suggesting the usage rather than content of the variable? Perhaps
DELIMITER_HBASE?



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

    Nit: Is this change necessary?



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

    Do you think that it would make sense to throw an exception here rather than returning
NULL?



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

    I would advise to abstract the composite key behaviour to the class parent and the the
parsing only once when the rowKey is being set rather than on each single row.


Jarcec

- Jarek Cecho


On May 10, 2013, 12:06 p.m., Shruti Joshi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11041/
> -----------------------------------------------------------
> 
> (Updated May 10, 2013, 12:06 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/java/org/apache/sqoop/hbase/ToStringPutTransformer.java 13c765c 
> 
> 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