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 <egyedb...@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
