accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request: Add Accumulo support to Sqoop
Date Tue, 25 Dec 2012 05:46:56 GMT

-----------------------------------------------------------
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:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2012, 1:57 p.m.)
> 
> 
> Review request for accumulo, Sqoop and Jarek Cecho.
> 
> 
> Description
> -------
> 
> Adds the ability to import to an Accumlo table in much the same manner as the current
HBase import capability.  Reported in JIRA as ACCUMULO-141 and SQOOP-767.
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/sqoop/SqoopOptions.java 613f797 
>   src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java PRE-CREATION 
>   src/java/org/apache/sqoop/accumulo/AccumuloUtil.java PRE-CREATION 
>   src/java/org/apache/sqoop/accumulo/MutationTransformer.java PRE-CREATION 
>   src/java/org/apache/sqoop/accumulo/ToStringMutationTransformer.java PRE-CREATION 
>   src/java/org/apache/sqoop/manager/SqlManager.java 3a52c6d 
>   src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/AccumuloImportMapper.java PRE-CREATION 
>   src/java/org/apache/sqoop/tool/BaseSqoopTool.java d795646 
>   src/java/org/apache/sqoop/tool/ImportTool.java 10f0cb9 
>   src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/8559/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Philip Grim
> 
>


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