----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14240/#review26840 -----------------------------------------------------------
Hi Alexandre, thank you very much for incorporating all the reviewers feedback, greatly appreciated! I do have couple of small nits below and one more major point - this will be user visible new feature, so we do need to update the documentation. You can find sources of the documentation in src/docs/user/ directory and you will be most likely interested in file hbase.txt. You can build the documentation using "ant docs" command. src/java/org/apache/sqoop/mapreduce/HBaseBulkImportJob.java <https://reviews.apache.org/r/14240/#comment52192> Nit: I do see couple of unused imports here, can we clean them up? src/java/org/apache/sqoop/mapreduce/HBaseBulkImportJob.java <https://reviews.apache.org/r/14240/#comment52193> Method isDirectory is not available in Hadoop 1 that we are still supporting, so we might need to change the method call to isDir() instead. src/java/org/apache/sqoop/tool/BaseSqoopTool.java <https://reviews.apache.org/r/14240/#comment52190> Nit: It seems that we are not using "enabled" keywords for boolean arguments. Do you think that we can use just "--hbase-bulkload" parameter instead? src/java/org/apache/sqoop/tool/BaseSqoopTool.java <https://reviews.apache.org/r/14240/#comment52191> The error seems to be indicating that both target directory and hbase table are required, however the condition seems to be checking only for at least one of them. Can we make sure that the condition is equivalent with the error message? src/java/org/apache/sqoop/tool/BaseSqoopTool.java <https://reviews.apache.org/r/14240/#comment52194> We are usually not requiring target directory specified as we have a logic to use one based on table name and or warehouse path: https://github.com/apache/sqoop/blob/trunk/src/java/org/apache/sqoop/tool/ImportTool.java#L454 I would suggest to reuse this concept. Jarcec - Jarek Cecho On Oct. 4, 2013, 5:45 p.m., Alexandre Normand wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14240/ > ----------------------------------------------------------- > > (Updated Oct. 4, 2013, 5:45 p.m.) > > > Review request for Sqoop, Jarek Cecho, Jean-Marc Spaggiari, and Vasanth kumar > RJ. > > > Repository: sqoop-trunk > > > Description > ------- > > This is the patch to address some of the comments on Zhancheng Deng's review > request (https://reviews.apache.org/r/13052/) which was basically the change > to add the --bulk-load-dir option to support the HBase doBulkLoad function. > > > Diffs > ----- > > src/java/org/apache/sqoop/SqoopOptions.java 01805f9 > src/java/org/apache/sqoop/hbase/HBasePutProcessor.java 9ceb5bd > src/java/org/apache/sqoop/hbase/ToStringPutTransformer.java 5ccf311 > src/java/org/apache/sqoop/manager/SqlManager.java 2a4992d > src/java/org/apache/sqoop/mapreduce/HBaseBulkImportJob.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/HBaseBulkImportMapper.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/ImportJobBase.java ab7f21e > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 0eca991 > src/test/com/cloudera/sqoop/TestSqoopOptions.java 03e2504 > > Diff: https://reviews.apache.org/r/14240/diff/ > > > Testing > ------- > > > Thanks, > > Alexandre Normand > >
