Jarek Cecho
2012-12-25, 05:46
Philip Grim
2013-01-10, 21:51
Philip Grim
2013-01-10, 22:23
Philip Grim
2013-01-10, 22:24
Jarek Cecho
2013-01-15, 08:04
Philip Grim
2013-01-17, 15:30
Philip Grim
2013-01-17, 15:30
-
Re: Review Request: Add Accumulo support to SqoopJarek Cecho 2012-12-25, 05:46
-----------------------------------------------------------
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:
-
Re: Review Request: Add Accumulo support to SqoopPhilip Grim 2013-01-10, 21:51
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Jan. 10, 2013, 9:51 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Updated the patch to address latest review comments.
Still need to add more tests - since successful testing is dependent on an installation of Accumulo and Zookeeper (or acceptable test stubs) this will take more time.
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 (updated)
-----
ivy.xml ca2b89b
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 92c9996
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 ea961cd
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 0c9cda6
src/java/org/apache/sqoop/tool/ImportTool.java 415b315
src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/8559/diff/
Testing
-------
Thanks,
Philip Grim
-
Re: Review Request: Add Accumulo support to SqoopPhilip Grim 2013-01-10, 22:23
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Jan. 10, 2013, 10:23 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Updated the patch to address latest review comments.
Still need to add more tests - since successful testing is dependent on an installation of Accumulo and Zookeeper (or acceptable test stubs) this will take more time.
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
-----
ivy.xml ca2b89b
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 92c9996
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 ea961cd
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 0c9cda6
src/java/org/apache/sqoop/tool/ImportTool.java 415b315
src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/8559/diff/
Testing
-------
Thanks,
Philip Grim
-
Re: Review Request: Add Accumulo support to SqoopPhilip Grim 2013-01-10, 22:24
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Jan. 10, 2013, 10:24 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Fixed typo in description.
Description (updated)
-------
Adds the ability to import to an Accumulo table in much the same manner as the current HBase import capability. Reported in JIRA as ACCUMULO-141 and SQOOP-767.
Diffs
-----
ivy.xml ca2b89b
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 92c9996
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 ea961cd
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 0c9cda6
src/java/org/apache/sqoop/tool/ImportTool.java 415b315
src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/8559/diff/
Testing
-------
Thanks,
Philip Grim
-
Re: Review Request: Add Accumulo support to SqoopJarek Cecho 2013-01-15, 08:04
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review15344
-----------------------------------------------------------
Hi Philip,
thank you very much for incorporating my suggestions. I believe that your patch is on the way to get in. Couple of notes:
1) I've noticed that new files are introducing some trailing white space characters, would you mind removing them? I've highlighted them for you in the review notes below.
2) I don't thing that we need installed Accumulo in order to perform basic functional tests. I'm expecting that Accumulo have something like HBase/Hadoop MiniCluster to make functional tests easier, so I would recommend to use that.
ivy.xml
<https://reviews.apache.org/r/8559/#comment33005>
Nit: Please remove the whitespace characters at the end.
src/docs/user/accumulo-args.txt
<https://reviews.apache.org/r/8559/#comment33010>
Nit: Please remove the whitespace characters at the end.
src/docs/user/accumulo-args.txt
<https://reviews.apache.org/r/8559/#comment33011>
Nit: Please remove the whitespace characters at the end.
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment33006>
Nit: Please remove the whitespace characters at the end.
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment33007>
Nit: Please remove the whitespace characters at the end.
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment33008>
Nit: Please remove the whitespace characters at the end.
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment33009>
Nit: Please remove the whitespace characters at the end.
Thank you for all your hard work!
Jarcec
- Jarek Cecho
On Jan. 10, 2013, 10:24 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2013, 10:24 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Description
> -------
>
> Adds the ability to import to an Accumulo table in much the same manner as the current HBase import capability. Reported in JIRA as ACCUMULO-141 and SQOOP-767.
>
>
> Diffs
> -----
>
> ivy.xml ca2b89b
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 92c9996
> 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 ea961cd
> 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 0c9cda6
> src/java/org/apache/sqoop/tool/ImportTool.java 415b315
> src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8559/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Philip Grim
>
>
-
Re: Review Request: Add Accumulo support to SqoopPhilip Grim 2013-01-17, 15:30
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Jan. 17, 2013, 3:30 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Updated patch to fix style nits.
Added JUnit testing using Accumulo MockInstance.
Description
-------
Adds the ability to import to an Accumulo table in much the same manner as the current HBase import capability. Reported in JIRA as ACCUMULO-141 and SQOOP-767.
Diffs (updated)
-----
ivy.xml ca2b89b
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 92c9996
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 ea961cd
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 0c9cda6
src/java/org/apache/sqoop/tool/ImportTool.java 415b315
src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java PRE-CREATION
src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java PRE-CREATION
src/test/org/apache/sqoop/accumulo/TestAccumuloQueryImport.java PRE-CREATION
src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/8559/diff/
Testing
-------
Thanks,
Philip Grim
-
Re: Review Request: Add Accumulo support to SqoopPhilip Grim 2013-01-17, 15:30
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Jan. 17, 2013, 3:30 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Description
-------
Adds the ability to import to an Accumulo table in much the same manner as the current HBase import capability. Reported in JIRA as ACCUMULO-141 and SQOOP-767.
Diffs
-----
ivy.xml ca2b89b
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 92c9996
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 ea961cd
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 0c9cda6
src/java/org/apache/sqoop/tool/ImportTool.java 415b315
src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java PRE-CREATION
src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java PRE-CREATION
src/test/org/apache/sqoop/accumulo/TestAccumuloQueryImport.java PRE-CREATION
src/test/org/apache/sqoop/accumulo/TestAccumuloUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/8559/diff/
Testing
-------
Thanks,
Philip Grim
