sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jilani Shaik <jilani2...@gmail.com>
Subject Re: Review Request 57499: Sqoop incremental import - NULL column updates are not pulled into HBase table
Date Sat, 18 Mar 2017 17:49:42 GMT
Hi Bogi,

Thanks for reviewing and suggestions. I will work on these.
Can you point the test class where the unit test for hbase transformer is, so that I can get
some help on unit test.

Thanks,
Jilani 

Sent from my iPhone

> On Mar 18, 2017, at 6:06 AM, Boglarka Egyed <egyedbogi@gmail.com> wrote:
> 
> 
> This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57499/
> 
> Hi Jilani,
> 
> Thanks for your contribution!
> 
> I have applied your patch and got some warnings because of the incorrect indentation
and trailing whitespaces which I mentioned in my review below too, please find my further
findings there too. I could run 'ant clean test' susseccfully with your patch.
> 
> On the top of the findings, could you please add a test case for your change?
> 
> Thanks,
> Bogi
> 
> src/java/org/apache/sqoop/hbase/HBasePutProcessor.java (Diff revision 1)
> 131	
> 134	
>     List<Mutation> putList = putTransformer.getPutCommand(fields);
> Could you please use a more conventional naming to make the code more self-explanatory?
I think put should be changed to mutation from now as you are working with mutations instead
of puts only. This applies to your whole change in every file too.
> 
> src/java/org/apache/sqoop/hbase/HBasePutProcessor.java (Diff revision 1)
> 136	
>           if (put.isEmpty()) {
> 138	
>         	if(put instanceof Put) {
> 139	
>         		Put putObject = (Put) put;
> 140	
>                 if (putObject.isEmpty()) {
> 137	
>             LOG.warn("Could not insert row with no columns "
> 141	
> >>>>>>            LOG.warn("Could not insert row with no columns "
> 138	
>                 + "for row-key column: " + Bytes.toString(put.getRow()));
> 142	
>                       + "for row-key column: " + Bytes.toString(putObject.getRow()));
> 139	
>           } else {
> 143	
> >>>>>>          } else {
> 140	
>             this.table.put(put);
> 144	
>                   this.table.put(putObject);
> 141	
>           }
> 145	
>                 }	
> 146	
>         	} else if(put instanceof Delete) {        		
> 147	
>             	Delete deleteObject = (Delete) put;
> 148	
>             	if (deleteObject.isEmpty()) {
> 149	
>                     LOG.warn("Could not delete row with no columns "
> 150	
>                         + "for row-key column: " + Bytes.toString(deleteObject.getRow()));
> 151	
>                   } else {
> 152	
>                   	this.table.delete(deleteObject);
> 153	
>                   }	
> 154	
>         	}        	
> Something with the indentation went wrong here, Sqoop uses 2 space indentation by default,
could you please correct it? Plus there are several trailing white spaces too (marked as red
rectangles in Diff).
> 
> src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java (Diff revision 1)
> 202	
>         	delete.deleteColumn(colFamilyBytes, getFieldNameBytes(colName));    	
> Please correct the indentation here too.
> 
> src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java (Diff revision 1)
> 210	
>     //return Collections.singletonList(put);    
> Could you please avoid to comment old lines out and remove them instead?
> 
> src/java/org/apache/sqoop/mapreduce/HBaseBulkImportMapper.java (Diff revision 1)
> 84	
>       context.write(new ImmutableBytesWritable(put.getRow()), put);
> 86	
>     	if(put != null && put instanceof Put) {
> 87	
>     		Put putObject = (Put) put;
> 88	
>     	    context.write(new ImmutableBytesWritable(putObject.getRow()), putObject);	
> Plese correct the indentation here too.
> 
> - Boglarka Egyed
> 
> 
> On March 11th, 2017, 6:16 a.m. UTC, Jilani Shaik wrote:
> 
> Review request for Sqoop.
> By Jilani Shaik.
> Updated March 11, 2017, 6:16 a.m.
> 
> Bugs: 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
> Testing
> 
> Executed with jar prepared with these changes into hadoop cluster and tested bot import
and then incremental import.
> 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)
> View Diff
> 
> File Attachments
> 
> HBase delete support for incremental import

Mime
  • Unnamed multipart/alternative (inline, 7-Bit, 0 bytes)
View raw message