Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Accumulo >> mail # dev >> Review Request: Add Accumulo support to Sqoop


+
Philip Grim 2012-12-12, 21:50
Copy link to this message
-
Re: Review Request: Add Accumulo support to Sqoop

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review14877
-----------------------------------------------------------
Hi Philip,
thank you very much for taking up this one. Please accept my apologies for this late review. I did not yet have chance to try your patch out on some testing environment as I firstly need to install the Accumulo database somewhere :-)

Few high level notes:

1) Please remove all trailing whitespace characters, I've marked them down here on review board.

2) Sqoop is using 2 space indentation across entire code base, would be great if you could update your patch  accordingly.

3) You're working on new big functionality, so it would be great if you could update the user guide as well. You can find user guide in src/docs/user.

4) Would you mind introducing couple of tests to make sure that everything works as expected?

5) Can you run "ant checkstyle" and fix all errors that shows up?
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment31772>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment31773>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment31774>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment31775>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment31776>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment31777>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment31792>

    Is null a safe default here? I'm concerned about NullPointerException(s) generated by this. It seems that ToSringMutationTransformer is not checking any of those for null values.

src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment31778>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/accumulo/MutationTransformer.java
<https://reviews.apache.org/r/8559/#comment31793>

    This seems to be an abstract class and not an interface.

src/java/org/apache/sqoop/accumulo/ToStringMutationTransformer.java
<https://reviews.apache.org/r/8559/#comment31794>

    Please use spaces instead of tabulator.

src/java/org/apache/sqoop/manager/SqlManager.java
<https://reviews.apache.org/r/8559/#comment31779>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment31807>

    Shouldn't you be using AccumuloImportMapper?

src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment31809>

    Shouldn't we be asking for Accumulo Row Key Column?

src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment31780>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment31781>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31782>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31783>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31784>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31785>

    

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31786>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31787>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31788>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31789>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31790>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31791>

    Nit: Please remove the whitespace characters at the end.

src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment31808>

    Can we add additional validations? For example --hbase-import and --accumulo-import are not compatible with each other, right?

src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment31810>

    I would suggest to change the "HBase" to "Accumulo" :-)
Thank you very much for all your hard work!

Jarcec

- Jarek Cecho
On Dec. 13, 2012, 1:57 p.m., Philip Grim wrote:
+
Philip Grim 2013-01-10, 21:51
+
Philip Grim 2013-01-10, 22:23
+
Philip Grim 2013-01-10, 22:24
+
Philip Grim 2013-01-17, 15:30
+
Philip Grim 2013-01-17, 15:30
+
Jarek Cecho 2013-01-15, 08:04