sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boglarka Egyed <egyedb...@gmail.com>
Subject Re: Review Request 57499: Sqoop incremental import - NULL column updates are not pulled into HBase table
Date Sun, 09 Apr 2017 11:23:40 GMT

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



Hi Jilani,

Thanks for adding an integration test case for your change. I ran 'ant clean test' and 'ant
clean test -Dthirdparty=true -Dsqoop.thirdparty.lib.dir=3rdparty -Dtestcase=HBaseImportTest'
with your patch successfully.

However, I still have some comments regarding your patch. I got warning during applying your
patch because of incorrect indentation and trailing whitespaces, please correct them - these
are marked with green arrows and red rectangles in Diff view which can help you. I also had
some findings related to the added test case, please find them below.

I would like to suggest to add some committers to 'People" as at least on of them needs to
review your change too, e.g. Anna Szonyi (szonyi) or Attila Szabo (maugli).

Thanks,
Bogi


src/test/com/cloudera/sqoop/hbase/HBaseImportTest.java
Lines 78 (patched)
<https://reviews.apache.org/r/57499/#comment244334>

    Could you please use more verbose naming here instead of "OverwriteT" and "OverwriteF".
It makes difficult to undersand the test case.



src/test/com/cloudera/sqoop/hbase/HBaseTestCase.java
Lines 171 (patched)
<https://reviews.apache.org/r/57499/#comment244332>

    I would suggest to use a parameter here too instead of a hardcoded value.



src/test/com/cloudera/sqoop/testutil/BaseSqoopTestCase.java
Lines 417 (patched)
<https://reviews.apache.org/r/57499/#comment244333>

    There is no colNames input parameter in the method signature.


- Boglarka Egyed


On March 31, 2017, 5:20 a.m., Jilani Shaik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57499/
> -----------------------------------------------------------
> 
> (Updated March 31, 2017, 5:20 a.m.)
> 
> 
> Review request for Sqoop and Boglarka Egyed.
> 
> 
> Bugs: SQOOP-3149
>     https://issues.apache.org/jira/browse/SQOOP-3149
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> HBase delete is added as part of incremental data import, Modified the return type of
method which is responsible for holding list of Put objects to List of Mutation objects and
at the time of execution of insert or delete using the type of object in Mutation, executed
the actaul operation either insert or delete.
> 
> Jira ticket for the same: https://issues.apache.org/jira/browse/SQOOP-3149
> 
> Similar ticket to above:  SQOOP-3125
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/hbase/HBasePutProcessor.java fdbe1276 
>   src/java/org/apache/sqoop/hbase/PutTransformer.java 533467e5 
>   src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java 363e1456 
>   src/java/org/apache/sqoop/mapreduce/HBaseBulkImportMapper.java 58ccee7b 
>   src/test/com/cloudera/sqoop/hbase/HBaseImportTest.java fa14a013 
>   src/test/com/cloudera/sqoop/hbase/HBaseTestCase.java a054eb66 
>   src/test/com/cloudera/sqoop/testutil/BaseSqoopTestCase.java 6310a398 
> 
> 
> Diff: https://reviews.apache.org/r/57499/diff/3/
> 
> 
> Testing
> -------
> 
> Executed with jar prepared with these changes into hadoop cluster and tested bot import
and then incremental import.
> 
> 
> File Attachments
> ----------------
> 
> HBase delete support for incremental import
>   https://reviews.apache.org/media/uploaded/files/2017/03/11/5b1895fd-4c6b-42fa-8a92-4e093153e370__hbase_delete_support_in_incremental_import
> updated with unit test and based on below suggestions
>   https://reviews.apache.org/media/uploaded/files/2017/03/22/708f63ba-2d8a-4a47-ab67-c1d2776354fd__hbase_delete_support_in_incremental_unit_test_also
> 
> 
> Thanks,
> 
> Jilani Shaik
> 
>


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