You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Philip Grim <ph...@insufficient-light.com> on 2012/12/12 22:50:32 UTC
Review Request: Add Accumulo support to Sqoop
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
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
Re: Review Request: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java, lines 42-58
> > <https://reviews.apache.org/r/8559/diff/5/?file=370079#file370079line42>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
>
> Philip Grim wrote:
> Apparently, I can't.
>
> My test cases are using the Sqoop-provided base test case, which extends JUnit test case. This appears to cause JUnit to only use JUnit 3 semantics when interpreting the @Test annotations, so the expects clause doesn't work.
>
> Philip Grim wrote:
> My suggestion is to drop this for now, and talk about refactoring the test cases in a more JUnit 4 compliant way in another issue. I'm open to alternate suggestions if you know a solution I'm missing.
Sounds good to me.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java, lines 105-122
> > <https://reviews.apache.org/r/8559/diff/5/?file=370079#file370079line105>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
>
> Philip Grim wrote:
> Same as above.
Sounds good to me.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloQueryImport.java, lines 50-68
> > <https://reviews.apache.org/r/8559/diff/5/?file=370080#file370080line50>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
>
> Philip Grim wrote:
> Same as above.
Sounds good to me.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review27458
-----------------------------------------------------------
On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:22 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java, lines 42-58
> > <https://reviews.apache.org/r/8559/diff/5/?file=370079#file370079line42>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
Apparently, I can't.
My test cases are using the Sqoop-provided base test case, which extends JUnit test case. This appears to cause JUnit to only use JUnit 3 semantics when interpreting the @Test annotations, so the expects clause doesn't work.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java, lines 105-122
> > <https://reviews.apache.org/r/8559/diff/5/?file=370079#file370079line105>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
Same as above.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloQueryImport.java, lines 50-68
> > <https://reviews.apache.org/r/8559/diff/5/?file=370080#file370080line50>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
Same as above.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review27458
-----------------------------------------------------------
On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:22 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java, lines 42-58
> > <https://reviews.apache.org/r/8559/diff/5/?file=370079#file370079line42>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
>
> Philip Grim wrote:
> Apparently, I can't.
>
> My test cases are using the Sqoop-provided base test case, which extends JUnit test case. This appears to cause JUnit to only use JUnit 3 semantics when interpreting the @Test annotations, so the expects clause doesn't work.
My suggestion is to drop this for now, and talk about refactoring the test cases in a more JUnit 4 compliant way in another issue. I'm open to alternate suggestions if you know a solution I'm missing.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review27458
-----------------------------------------------------------
On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:22 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java, lines 42-58
> > <https://reviews.apache.org/r/8559/diff/5/?file=370079#file370079line42>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
>
> Philip Grim wrote:
> Apparently, I can't.
>
> My test cases are using the Sqoop-provided base test case, which extends JUnit test case. This appears to cause JUnit to only use JUnit 3 semantics when interpreting the @Test annotations, so the expects clause doesn't work.
My suggestion is to drop this for now, and talk about refactoring the test cases in a more JUnit 4 compliant way in another issue. I'm open to alternate suggestions if you know a solution I'm missing.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review27458
-----------------------------------------------------------
On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:22 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java, lines 42-58
> > <https://reviews.apache.org/r/8559/diff/5/?file=370079#file370079line42>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
Apparently, I can't.
My test cases are using the Sqoop-provided base test case, which extends JUnit test case. This appears to cause JUnit to only use JUnit 3 semantics when interpreting the @Test annotations, so the expects clause doesn't work.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java, lines 105-122
> > <https://reviews.apache.org/r/8559/diff/5/?file=370079#file370079line105>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
Same as above.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloQueryImport.java, lines 50-68
> > <https://reviews.apache.org/r/8559/diff/5/?file=370080#file370080line50>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
Same as above.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review27458
-----------------------------------------------------------
On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:22 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java, lines 42-58
> > <https://reviews.apache.org/r/8559/diff/5/?file=370079#file370079line42>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
>
> Philip Grim wrote:
> Apparently, I can't.
>
> My test cases are using the Sqoop-provided base test case, which extends JUnit test case. This appears to cause JUnit to only use JUnit 3 semantics when interpreting the @Test annotations, so the expects clause doesn't work.
>
> Philip Grim wrote:
> My suggestion is to drop this for now, and talk about refactoring the test cases in a more JUnit 4 compliant way in another issue. I'm open to alternate suggestions if you know a solution I'm missing.
Sounds good to me.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java, lines 105-122
> > <https://reviews.apache.org/r/8559/diff/5/?file=370079#file370079line105>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
>
> Philip Grim wrote:
> Same as above.
Sounds good to me.
> On Oct. 24, 2013, 5:09 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/TestAccumuloQueryImport.java, lines 50-68
> > <https://reviews.apache.org/r/8559/diff/5/?file=370080#file370080line50>
> >
> > Could you rewrite this to use the @Test(expects=IOException.class) form?
>
> Philip Grim wrote:
> Same as above.
Sounds good to me.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review27458
-----------------------------------------------------------
On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:22 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review27458
-----------------------------------------------------------
src/docs/user/accumulo-args.txt
<https://reviews.apache.org/r/8559/#comment53357>
Similar to above.
"Import into a test Accumulo instance"
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment53358>
Could you change this to take an argument for a single visibility to apply to everything?
Or would you prefer I make it a follow on issue?
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment53359>
a brief bit here about what information I need in order to connect things to my Accumulo instance would help.
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment53355>
Since these docs are geared towards end users, I'd recommend phrasing similar to:
"The Accumulo import tool has a testing mode, activated with the --accumulo-test-mode flag. When run in testing mode, the import spins up a small test instance of Accumulo rather than writing to an existing cluster."
If a paragraph is added above about ocnnecting to an Accumulo instance, then also add a sentence that the zookeeper and instance name aren't needed.
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment53374>
The settings for batch size and max latency (and maybe threads) should be optional command line args, since they can have a large impact on performance.
These settings (2GB and 5 seconds) don't seem like appropriate defaults.
for the max latency a default of 0 or Long.MAX_VALUE would be preferable, presuming Sqoop is going for high throughput.
for max memory buffer to hold before writing, a few MB or 10s of MB should be sufficient.
Might also want to switch to the non-deprecated createBatchWriter method.
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment53372>
Instead of wrapping in RuntimeException, consider IOException
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment53371>
Pending mutations can get rejected for reasons other than the table not being found.
Since this is a failure to write, probably better to wrap in an IOException instead of RuntimeException.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment53362>
This should use MiniAccumuloCluster instead of MockInstance.
http://accumulo.apache.org/1.5/accumulo_user_manual.html#_mini_accumulo_cluster
It gives a much more realistic test environment for how Accumulo will behave.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment53363>
Logging at a unimportant level is preferable to swallowing.
LOG.debug or LOG.info?
When doing ops, I definitely care if some other process created the table while I was telling this process to do it.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment53365>
Please don't include format fixes unrelated to this patch.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment53366>
Please don't include format fixes unrelated to this patch.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment53367>
Please don't include format fixes unrelated to this patch.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment53368>
Avro and Sequence file arguments don't work with Accumulo as well, I think?
src/java/org/apache/sqoop/tool/ImportTool.java
<https://reviews.apache.org/r/8559/#comment53369>
nit: trailing whitespace
src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java
<https://reviews.apache.org/r/8559/#comment53375>
why this logic for only running on Hadoop 0.20.x?
Presuming keeping these only on hadoop 0.20 is needed, does ant have something analogous to Maven profiles that could instead isolate these tests to only run given a specific hadoop version?
As is, this logic will falsely inflate the number of passing tests when they're actually being ignored.
src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java
<https://reviews.apache.org/r/8559/#comment53379>
I'd like to see test cases for dealing with the failure path on writing and on close, but I can't think of a good way to do that with the current Accumulo offerings. :/
src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java
<https://reviews.apache.org/r/8559/#comment53376>
Could you rewrite this to use the @Test(expects=IOException.class) form?
src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java
<https://reviews.apache.org/r/8559/#comment53377>
Could you rewrite this to use the @Test(expects=IOException.class) form?
src/test/org/apache/sqoop/accumulo/TestAccumuloQueryImport.java
<https://reviews.apache.org/r/8559/#comment53378>
Could you rewrite this to use the @Test(expects=IOException.class) form?
- Sean Busbey
On Oct. 23, 2013, 9:24 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2013, 9:24 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review27458
-----------------------------------------------------------
src/docs/user/accumulo-args.txt
<https://reviews.apache.org/r/8559/#comment53357>
Similar to above.
"Import into a test Accumulo instance"
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment53358>
Could you change this to take an argument for a single visibility to apply to everything?
Or would you prefer I make it a follow on issue?
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment53359>
a brief bit here about what information I need in order to connect things to my Accumulo instance would help.
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment53355>
Since these docs are geared towards end users, I'd recommend phrasing similar to:
"The Accumulo import tool has a testing mode, activated with the --accumulo-test-mode flag. When run in testing mode, the import spins up a small test instance of Accumulo rather than writing to an existing cluster."
If a paragraph is added above about ocnnecting to an Accumulo instance, then also add a sentence that the zookeeper and instance name aren't needed.
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment53374>
The settings for batch size and max latency (and maybe threads) should be optional command line args, since they can have a large impact on performance.
These settings (2GB and 5 seconds) don't seem like appropriate defaults.
for the max latency a default of 0 or Long.MAX_VALUE would be preferable, presuming Sqoop is going for high throughput.
for max memory buffer to hold before writing, a few MB or 10s of MB should be sufficient.
Might also want to switch to the non-deprecated createBatchWriter method.
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment53372>
Instead of wrapping in RuntimeException, consider IOException
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment53371>
Pending mutations can get rejected for reasons other than the table not being found.
Since this is a failure to write, probably better to wrap in an IOException instead of RuntimeException.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment53362>
This should use MiniAccumuloCluster instead of MockInstance.
http://accumulo.apache.org/1.5/accumulo_user_manual.html#_mini_accumulo_cluster
It gives a much more realistic test environment for how Accumulo will behave.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment53363>
Logging at a unimportant level is preferable to swallowing.
LOG.debug or LOG.info?
When doing ops, I definitely care if some other process created the table while I was telling this process to do it.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment53365>
Please don't include format fixes unrelated to this patch.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment53366>
Please don't include format fixes unrelated to this patch.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment53367>
Please don't include format fixes unrelated to this patch.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment53368>
Avro and Sequence file arguments don't work with Accumulo as well, I think?
src/java/org/apache/sqoop/tool/ImportTool.java
<https://reviews.apache.org/r/8559/#comment53369>
nit: trailing whitespace
src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java
<https://reviews.apache.org/r/8559/#comment53375>
why this logic for only running on Hadoop 0.20.x?
Presuming keeping these only on hadoop 0.20 is needed, does ant have something analogous to Maven profiles that could instead isolate these tests to only run given a specific hadoop version?
As is, this logic will falsely inflate the number of passing tests when they're actually being ignored.
src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java
<https://reviews.apache.org/r/8559/#comment53379>
I'd like to see test cases for dealing with the failure path on writing and on close, but I can't think of a good way to do that with the current Accumulo offerings. :/
src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java
<https://reviews.apache.org/r/8559/#comment53376>
Could you rewrite this to use the @Test(expects=IOException.class) form?
src/test/org/apache/sqoop/accumulo/TestAccumuloImport.java
<https://reviews.apache.org/r/8559/#comment53377>
Could you rewrite this to use the @Test(expects=IOException.class) form?
src/test/org/apache/sqoop/accumulo/TestAccumuloQueryImport.java
<https://reviews.apache.org/r/8559/#comment53378>
Could you rewrite this to use the @Test(expects=IOException.class) form?
- Sean Busbey
On Oct. 23, 2013, 9:24 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2013, 9:24 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28271
-----------------------------------------------------------
Hi Philip,
I do have couple of nits:
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment55042>
Nit: I believe that there is missing dash in the "--accumulo password" parameter?
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment55043>
Nit: Unnecessary //
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment55044>
Nit: Please put space between "//" and the comment itself.
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment55045>
Nit: Please put space between "//" and the comment itself.
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment55046>
Nit: Please put space between "//" and the comment itself.
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment55047>
Nit: Please put space between "//" and the comment itself.
src/java/org/apache/sqoop/accumulo/AccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment55263>
Nit: Can we put the class name into constant?
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment55276>
Nit: Wouldn't be simpler to verify that FileLayout have to be the default one rather then verifying every other possibility?
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment55277>
Would it be valid situation for the password to be missing?
Jarcec
- Jarek Cecho
On Nov. 5, 2013, 9:11 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2013, 9:11 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28593
-----------------------------------------------------------
Hi Philip,
I was playing with the patch a bit and I have to say, very good job! Unit tests in all profiles are passing, which is great. I'm still trying to test the patch on a real Hadoop/Accumulo cluster. I'll keep you posted about my progress there. Couple of notes that I've noticed:
1) You've put together great docs that are very easy to follow, thank you! They are however not being included in user guide as they are referenced anywhere. Would you mind ensuring that the docs will get propagated into the User guide as well? You can use command ant docs to generate the User guide.
2) For Hive and HBase import we do have special bash code in the loading scripts that are ensuring that all the required dependencies are on classpath. Would you mind providing similar solution for Accumulo, so that users don't have to hack the classpath together themselves?
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment55439>
Seems like unused import.
Jarcec
- Jarek Cecho
On Nov. 11, 2013, 3:31 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2013, 3:31 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Nov. 19, 2013, 9:02 p.m., Jarek Cecho wrote:
> > Thank you for following up Philip!
> >
> > I run tests on all supported Hadoop profiles and it seems to be working! I'm however having difficulties with real cluster tests, where I'm getting:
> >
> > Error: java.lang.ClassNotFoundException: org.apache.accumulo.core.client.MutationsRejectedException
> > at java.net.URLClassLoader$1.run(URLClassLoader.java:202)
> > at java.security.AccessController.doPrivileged(Native Method)
> > at java.net.URLClassLoader.findClass(URLClassLoader.java:190)
> > at java.lang.ClassLoader.loadClass(ClassLoader.java:306)
> > at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301)
> > at java.lang.ClassLoader.loadClass(ClassLoader.java:247)
> > at java.lang.Class.forName0(Native Method)
> > at java.lang.Class.forName(Class.java:247)
> > at org.apache.hadoop.conf.Configuration.getClassByNameOrNull(Configuration.java:1526)
> > at org.apache.hadoop.conf.Configuration.getClassByName(Configuration.java:1491)
> > at org.apache.hadoop.conf.Configuration.getClass(Configuration.java:1585)
> > at org.apache.sqoop.mapreduce.DelegatingOutputFormat$DelegatingRecordWriter.<init>(DelegatingOutputFormat.java:104)
> > at org.apache.sqoop.mapreduce.DelegatingOutputFormat.getRecordWriter(DelegatingOutputFormat.java:82)
> > at org
> >
> > Sqoop do not propagate it's entire classpath to the mapreduce job, instead it's the connector/tool/job that specifies which jars needs to be propagated. For example HCatalog integration is having a util method [1], HBase integration is using special HBase API [2]. I think that the accumulo job have to do something similar.
> >
> > Links:
> > 1: https://github.com/apache/sqoop/blob/trunk/src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java#L727
> > 2: https://github.com/apache/sqoop/blob/trunk/src/java/org/apache/sqoop/mapreduce/HBaseImportJob.java#L228
> >
Yes, this isn't something I would notice, because in all of my Accumulo installations, I add the Accumulo and Zookeeper jars to the Hadoop classpath so that I don't have to specify all those jars every time I want to run a Map/Reduce job.
I'm posting the patch with the code to add the libjars, however I still need to do some testing.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review29129
-----------------------------------------------------------
On Nov. 18, 2013, 10:16 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 10:16 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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
> -----
>
> bin/configure-sqoop e604197
> bin/configure-sqoop.cmd ec57e37
> ivy.xml c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/docs/user/import.txt dfc9b39
> src/docs/user/validation.txt 282cfd6
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Nov. 19, 2013, 9:02 p.m., Jarek Cecho wrote:
> > Thank you for following up Philip!
> >
> > I run tests on all supported Hadoop profiles and it seems to be working! I'm however having difficulties with real cluster tests, where I'm getting:
> >
> > Error: java.lang.ClassNotFoundException: org.apache.accumulo.core.client.MutationsRejectedException
> > at java.net.URLClassLoader$1.run(URLClassLoader.java:202)
> > at java.security.AccessController.doPrivileged(Native Method)
> > at java.net.URLClassLoader.findClass(URLClassLoader.java:190)
> > at java.lang.ClassLoader.loadClass(ClassLoader.java:306)
> > at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301)
> > at java.lang.ClassLoader.loadClass(ClassLoader.java:247)
> > at java.lang.Class.forName0(Native Method)
> > at java.lang.Class.forName(Class.java:247)
> > at org.apache.hadoop.conf.Configuration.getClassByNameOrNull(Configuration.java:1526)
> > at org.apache.hadoop.conf.Configuration.getClassByName(Configuration.java:1491)
> > at org.apache.hadoop.conf.Configuration.getClass(Configuration.java:1585)
> > at org.apache.sqoop.mapreduce.DelegatingOutputFormat$DelegatingRecordWriter.<init>(DelegatingOutputFormat.java:104)
> > at org.apache.sqoop.mapreduce.DelegatingOutputFormat.getRecordWriter(DelegatingOutputFormat.java:82)
> > at org
> >
> > Sqoop do not propagate it's entire classpath to the mapreduce job, instead it's the connector/tool/job that specifies which jars needs to be propagated. For example HCatalog integration is having a util method [1], HBase integration is using special HBase API [2]. I think that the accumulo job have to do something similar.
> >
> > Links:
> > 1: https://github.com/apache/sqoop/blob/trunk/src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java#L727
> > 2: https://github.com/apache/sqoop/blob/trunk/src/java/org/apache/sqoop/mapreduce/HBaseImportJob.java#L228
> >
Yes, this isn't something I would notice, because in all of my Accumulo installations, I add the Accumulo and Zookeeper jars to the Hadoop classpath so that I don't have to specify all those jars every time I want to run a Map/Reduce job.
I'm posting the patch with the code to add the libjars, however I still need to do some testing.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review29129
-----------------------------------------------------------
On Nov. 18, 2013, 10:16 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 10:16 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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
> -----
>
> bin/configure-sqoop e604197
> bin/configure-sqoop.cmd ec57e37
> ivy.xml c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/docs/user/import.txt dfc9b39
> src/docs/user/validation.txt 282cfd6
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review29129
-----------------------------------------------------------
Thank you for following up Philip!
I run tests on all supported Hadoop profiles and it seems to be working! I'm however having difficulties with real cluster tests, where I'm getting:
Error: java.lang.ClassNotFoundException: org.apache.accumulo.core.client.MutationsRejectedException
at java.net.URLClassLoader$1.run(URLClassLoader.java:202)
at java.security.AccessController.doPrivileged(Native Method)
at java.net.URLClassLoader.findClass(URLClassLoader.java:190)
at java.lang.ClassLoader.loadClass(ClassLoader.java:306)
at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301)
at java.lang.ClassLoader.loadClass(ClassLoader.java:247)
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:247)
at org.apache.hadoop.conf.Configuration.getClassByNameOrNull(Configuration.java:1526)
at org.apache.hadoop.conf.Configuration.getClassByName(Configuration.java:1491)
at org.apache.hadoop.conf.Configuration.getClass(Configuration.java:1585)
at org.apache.sqoop.mapreduce.DelegatingOutputFormat$DelegatingRecordWriter.<init>(DelegatingOutputFormat.java:104)
at org.apache.sqoop.mapreduce.DelegatingOutputFormat.getRecordWriter(DelegatingOutputFormat.java:82)
at org
Sqoop do not propagate it's entire classpath to the mapreduce job, instead it's the connector/tool/job that specifies which jars needs to be propagated. For example HCatalog integration is having a util method [1], HBase integration is using special HBase API [2]. I think that the accumulo job have to do something similar.
Links:
1: https://github.com/apache/sqoop/blob/trunk/src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java#L727
2: https://github.com/apache/sqoop/blob/trunk/src/java/org/apache/sqoop/mapreduce/HBaseImportJob.java#L228
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment56257>
the accumulo-max-latency parameter is not closed with '+' sign.
Jarcec
- Jarek Cecho
On Nov. 18, 2013, 10:16 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 10:16 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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
> -----
>
> bin/configure-sqoop e604197
> bin/configure-sqoop.cmd ec57e37
> ivy.xml c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/docs/user/import.txt dfc9b39
> src/docs/user/validation.txt 282cfd6
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review29129
-----------------------------------------------------------
Thank you for following up Philip!
I run tests on all supported Hadoop profiles and it seems to be working! I'm however having difficulties with real cluster tests, where I'm getting:
Error: java.lang.ClassNotFoundException: org.apache.accumulo.core.client.MutationsRejectedException
at java.net.URLClassLoader$1.run(URLClassLoader.java:202)
at java.security.AccessController.doPrivileged(Native Method)
at java.net.URLClassLoader.findClass(URLClassLoader.java:190)
at java.lang.ClassLoader.loadClass(ClassLoader.java:306)
at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301)
at java.lang.ClassLoader.loadClass(ClassLoader.java:247)
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:247)
at org.apache.hadoop.conf.Configuration.getClassByNameOrNull(Configuration.java:1526)
at org.apache.hadoop.conf.Configuration.getClassByName(Configuration.java:1491)
at org.apache.hadoop.conf.Configuration.getClass(Configuration.java:1585)
at org.apache.sqoop.mapreduce.DelegatingOutputFormat$DelegatingRecordWriter.<init>(DelegatingOutputFormat.java:104)
at org.apache.sqoop.mapreduce.DelegatingOutputFormat.getRecordWriter(DelegatingOutputFormat.java:82)
at org
Sqoop do not propagate it's entire classpath to the mapreduce job, instead it's the connector/tool/job that specifies which jars needs to be propagated. For example HCatalog integration is having a util method [1], HBase integration is using special HBase API [2]. I think that the accumulo job have to do something similar.
Links:
1: https://github.com/apache/sqoop/blob/trunk/src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java#L727
2: https://github.com/apache/sqoop/blob/trunk/src/java/org/apache/sqoop/mapreduce/HBaseImportJob.java#L228
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment56257>
the accumulo-max-latency parameter is not closed with '+' sign.
Jarcec
- Jarek Cecho
On Nov. 18, 2013, 10:16 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 10:16 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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
> -----
>
> bin/configure-sqoop e604197
> bin/configure-sqoop.cmd ec57e37
> ivy.xml c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/docs/user/import.txt dfc9b39
> src/docs/user/validation.txt 282cfd6
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Nov. 20, 2013, 7:01 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloUtil.java, line 80
> > <https://reviews.apache.org/r/8559/diff/11-12/?file=388121#file388121line80>
> >
> > Nit: The return here is not relevant, right? Can we either put the "return" to the previous if-branch as well or remove it from here? It will be easier for others to see the similar code then wondering why there is a return on one place and not on the another :-)
You are correct, sir. This should not have been here.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review29173
-----------------------------------------------------------
On Nov. 19, 2013, 11:06 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2013, 11:06 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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
> -----
>
> bin/configure-sqoop e604197
> bin/configure-sqoop.cmd ec57e37
> ivy.xml c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/docs/user/import.txt dfc9b39
> src/docs/user/validation.txt 282cfd6
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Nov. 20, 2013, 7:01 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloUtil.java, line 80
> > <https://reviews.apache.org/r/8559/diff/11-12/?file=388121#file388121line80>
> >
> > Nit: The return here is not relevant, right? Can we either put the "return" to the previous if-branch as well or remove it from here? It will be easier for others to see the similar code then wondering why there is a return on one place and not on the another :-)
You are correct, sir. This should not have been here.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review29173
-----------------------------------------------------------
On Nov. 19, 2013, 11:06 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2013, 11:06 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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
> -----
>
> bin/configure-sqoop e604197
> bin/configure-sqoop.cmd ec57e37
> ivy.xml c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/docs/user/import.txt dfc9b39
> src/docs/user/validation.txt 282cfd6
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review29173
-----------------------------------------------------------
Ship it!
Hi Philip,
I was able to test the code on my Accumulo 1.5 cluster, good job! Thank you very much for following up through all the review rounds, I appreciate your patience! I do have couple of small final nits, so please fix those and upload the patch to the JIRA. I'll commit it shortly after that!
src/java/org/apache/sqoop/accumulo/AccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment56313>
Nit: The return here is not relevant, right? Can we either put the "return" to the previous if-branch as well or remove it from here? It will be easier for others to see the similar code then wondering why there is a return on one place and not on the another :-)
src/java/org/apache/sqoop/accumulo/AccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment56314>
Nit: Trailing white space.
bin/configure-sqoop
<https://reviews.apache.org/r/8559/#comment56322>
Since we are using the environment variable ACCUMULO_HOME in the JAVA application, we need to export it here. Otherwise it won't get propagated.
src/java/org/apache/sqoop/accumulo/AccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment56321>
Nit: Trailing white space
Jarcec
- Jarek Cecho
On Nov. 19, 2013, 11:06 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2013, 11:06 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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
> -----
>
> bin/configure-sqoop e604197
> bin/configure-sqoop.cmd ec57e37
> ivy.xml c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/docs/user/import.txt dfc9b39
> src/docs/user/validation.txt 282cfd6
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review29191
-----------------------------------------------------------
Ship it!
Ship It!
- Jarek Cecho
On Nov. 20, 2013, 8:26 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2013, 8:26 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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
> -----
>
> bin/configure-sqoop e604197
> bin/configure-sqoop.cmd ec57e37
> ivy.xml c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/docs/user/import.txt dfc9b39
> src/docs/user/validation.txt 282cfd6
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review29191
-----------------------------------------------------------
Ship it!
Ship It!
- Jarek Cecho
On Nov. 20, 2013, 8:26 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2013, 8:26 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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
> -----
>
> bin/configure-sqoop e604197
> bin/configure-sqoop.cmd ec57e37
> ivy.xml c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/docs/user/import.txt dfc9b39
> src/docs/user/validation.txt 282cfd6
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 20, 2013, 8:26 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Final version.
Repository: sqoop-trunk
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)
-----
bin/configure-sqoop e604197
bin/configure-sqoop.cmd ec57e37
ivy.xml c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/docs/user/import.txt dfc9b39
src/docs/user/validation.txt 282cfd6
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 20, 2013, 8:26 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Final version.
Repository: sqoop-trunk
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)
-----
bin/configure-sqoop e604197
bin/configure-sqoop.cmd ec57e37
ivy.xml c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/docs/user/import.txt dfc9b39
src/docs/user/validation.txt 282cfd6
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review29173
-----------------------------------------------------------
Ship it!
Hi Philip,
I was able to test the code on my Accumulo 1.5 cluster, good job! Thank you very much for following up through all the review rounds, I appreciate your patience! I do have couple of small final nits, so please fix those and upload the patch to the JIRA. I'll commit it shortly after that!
src/java/org/apache/sqoop/accumulo/AccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment56313>
Nit: The return here is not relevant, right? Can we either put the "return" to the previous if-branch as well or remove it from here? It will be easier for others to see the similar code then wondering why there is a return on one place and not on the another :-)
src/java/org/apache/sqoop/accumulo/AccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment56314>
Nit: Trailing white space.
bin/configure-sqoop
<https://reviews.apache.org/r/8559/#comment56322>
Since we are using the environment variable ACCUMULO_HOME in the JAVA application, we need to export it here. Otherwise it won't get propagated.
src/java/org/apache/sqoop/accumulo/AccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment56321>
Nit: Trailing white space
Jarcec
- Jarek Cecho
On Nov. 19, 2013, 11:06 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2013, 11:06 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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
> -----
>
> bin/configure-sqoop e604197
> bin/configure-sqoop.cmd ec57e37
> ivy.xml c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/docs/user/import.txt dfc9b39
> src/docs/user/validation.txt 282cfd6
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 19, 2013, 11:06 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Another update.
Repository: sqoop-trunk
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)
-----
bin/configure-sqoop e604197
bin/configure-sqoop.cmd ec57e37
ivy.xml c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/docs/user/import.txt dfc9b39
src/docs/user/validation.txt 282cfd6
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 19, 2013, 11:06 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Another update.
Repository: sqoop-trunk
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)
-----
bin/configure-sqoop e604197
bin/configure-sqoop.cmd ec57e37
ivy.xml c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/docs/user/import.txt dfc9b39
src/docs/user/validation.txt 282cfd6
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 18, 2013, 10:16 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Repository: sqoop-trunk
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
-----
bin/configure-sqoop e604197
bin/configure-sqoop.cmd ec57e37
ivy.xml c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/docs/user/import.txt dfc9b39
src/docs/user/validation.txt 282cfd6
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 18, 2013, 10:16 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Repository: sqoop-trunk
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
-----
bin/configure-sqoop e604197
bin/configure-sqoop.cmd ec57e37
ivy.xml c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/docs/user/import.txt dfc9b39
src/docs/user/validation.txt 282cfd6
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 18, 2013, 10:15 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Fixed unused import.
Added classpath info to configure scripts.
Added proper reference in user docs.
Repository: sqoop-trunk
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)
-----
bin/configure-sqoop e604197
bin/configure-sqoop.cmd ec57e37
ivy.xml c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/docs/user/import.txt dfc9b39
src/docs/user/validation.txt 282cfd6
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 18, 2013, 10:15 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Fixed unused import.
Added classpath info to configure scripts.
Added proper reference in user docs.
Repository: sqoop-trunk
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)
-----
bin/configure-sqoop e604197
bin/configure-sqoop.cmd ec57e37
ivy.xml c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/docs/user/import.txt dfc9b39
src/docs/user/validation.txt 282cfd6
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28593
-----------------------------------------------------------
Hi Philip,
I was playing with the patch a bit and I have to say, very good job! Unit tests in all profiles are passing, which is great. I'm still trying to test the patch on a real Hadoop/Accumulo cluster. I'll keep you posted about my progress there. Couple of notes that I've noticed:
1) You've put together great docs that are very easy to follow, thank you! They are however not being included in user guide as they are referenced anywhere. Would you mind ensuring that the docs will get propagated into the User guide as well? You can use command ant docs to generate the User guide.
2) For Hive and HBase import we do have special bash code in the loading scripts that are ensuring that all the required dependencies are on classpath. Would you mind providing similar solution for Accumulo, so that users don't have to hack the classpath together themselves?
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment55439>
Seems like unused import.
Jarcec
- Jarek Cecho
On Nov. 11, 2013, 3:31 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2013, 3:31 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 11, 2013, 3:31 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Updated with latest review board fixes.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28271
-----------------------------------------------------------
Hi Philip,
I do have couple of nits:
src/docs/user/accumulo.txt
<https://reviews.apache.org/r/8559/#comment55042>
Nit: I believe that there is missing dash in the "--accumulo password" parameter?
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment55043>
Nit: Unnecessary //
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment55044>
Nit: Please put space between "//" and the comment itself.
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment55045>
Nit: Please put space between "//" and the comment itself.
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment55046>
Nit: Please put space between "//" and the comment itself.
src/java/org/apache/sqoop/SqoopOptions.java
<https://reviews.apache.org/r/8559/#comment55047>
Nit: Please put space between "//" and the comment itself.
src/java/org/apache/sqoop/accumulo/AccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment55263>
Nit: Can we put the class name into constant?
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment55276>
Nit: Wouldn't be simpler to verify that FileLayout have to be the default one rather then verifying every other possibility?
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment55277>
Would it be valid situation for the password to be missing?
Jarcec
- Jarek Cecho
On Nov. 5, 2013, 9:11 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2013, 9:11 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
> 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 1ffa40f
> 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 018d11f
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 11, 2013, 3:31 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Updated with latest review board fixes.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
src/java/org/apache/sqoop/accumulo/AccumuloConstants.java PRE-CREATION
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 5, 2013, 9:11 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Someone committed between my pull and my last patch. Fixed it up so it applies cleanly again, and added UTF-8 encoding to the getBytes() calls in ToStringMutationProcessor.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 5, 2013, 9:11 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Someone committed between my pull and my last patch. Fixed it up so it applies cleanly again, and added UTF-8 encoding to the getBytes() calls in ToStringMutationProcessor.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 5c7a56a
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 1ffa40f
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 018d11f
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28223
-----------------------------------------------------------
looks good.
src/java/org/apache/sqoop/accumulo/ToStringMutationTransformer.java
<https://reviews.apache.org/r/8559/#comment54900>
It's worth a follow on ticket to control the character set rather than relying on the platform default, since it could vary across clients. FWIW, HBase relies on the hbase.util.Bytes class to specify UTF-8.
- Sean Busbey
On Nov. 5, 2013, 8:13 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2013, 8:13 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28223
-----------------------------------------------------------
looks good.
src/java/org/apache/sqoop/accumulo/ToStringMutationTransformer.java
<https://reviews.apache.org/r/8559/#comment54900>
It's worth a follow on ticket to control the character set rather than relying on the platform default, since it could vary across clients. FWIW, HBase relies on the hbase.util.Bytes class to specify UTF-8.
- Sean Busbey
On Nov. 5, 2013, 8:13 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2013, 8:13 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 5, 2013, 8:13 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Newest review changes.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 13637b5
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 1ffa40f
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 9230f82
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Nov. 5, 2013, 6:11 a.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java, lines 156-163
> > <https://reviews.apache.org/r/8559/diff/7/?file=377378#file377378line156>
> >
> > This will make sure we only attempt setupCluster once concurrently, but it won't make parallel tests wait for the cluster to finish.
> >
> > Why don't we mimic the HBase tests, and just spin up and take down the mini cluster for each test. Does that take too long?
> >
> > If a minicluster per test does take too long, I'd say it's not worth building the complexity to handle parallel tests correctly when e.g. the hbase tests don't. Just switch to a non-concurrent int (to keep the cleanup in @After) so it's obviously not threadsafe when future maintainers look at it.
> >
> > (sorry for the thrashing)
Too long is kind of subjective. I have it spinning up for each test now, and the TestAccumuloImport suite takes 34 seconds in a VM on my laptop. I don't know if that's too long or not, but I'm going with it.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28173
-----------------------------------------------------------
On Nov. 4, 2013, 11 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2013, 11 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Nov. 5, 2013, 6:11 a.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java, lines 156-163
> > <https://reviews.apache.org/r/8559/diff/7/?file=377378#file377378line156>
> >
> > This will make sure we only attempt setupCluster once concurrently, but it won't make parallel tests wait for the cluster to finish.
> >
> > Why don't we mimic the HBase tests, and just spin up and take down the mini cluster for each test. Does that take too long?
> >
> > If a minicluster per test does take too long, I'd say it's not worth building the complexity to handle parallel tests correctly when e.g. the hbase tests don't. Just switch to a non-concurrent int (to keep the cleanup in @After) so it's obviously not threadsafe when future maintainers look at it.
> >
> > (sorry for the thrashing)
Too long is kind of subjective. I have it spinning up for each test now, and the TestAccumuloImport suite takes 34 seconds in a VM on my laptop. I don't know if that's too long or not, but I'm going with it.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28173
-----------------------------------------------------------
On Nov. 4, 2013, 11 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2013, 11 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28173
-----------------------------------------------------------
Ship it!
Thanks again for all the hard work Philip!
Just a couple of minor things. Sorry I missed some of them in a previous review. I don't anticipate anything after these.
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment54808>
Recommend using Configuration.getLong instead of parsing yourself.
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment54809>
Same as above, re Configuration.getLong
src/java/org/apache/sqoop/accumulo/AccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment54811>
Just a note to myself to file a follow on issue for this testing flag in Accumulo and HBase.
They should probably be removed. If not, they should be package protected, since they're only needed in testing. also they should have a thread safety warning.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54813>
Missed an updated file that references AccumuloUtil for constants?
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54817>
Need to also pass MAX_LATENCY and BATCH_SIZE through to Configuration.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54815>
same as above, should be AccumuloUtil
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54814>
Same as above, should be AccumuloUtil
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54816>
same as above, should be AccumuloUtil
src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java
<https://reviews.apache.org/r/8559/#comment54812>
This will make sure we only attempt setupCluster once concurrently, but it won't make parallel tests wait for the cluster to finish.
Why don't we mimic the HBase tests, and just spin up and take down the mini cluster for each test. Does that take too long?
If a minicluster per test does take too long, I'd say it's not worth building the complexity to handle parallel tests correctly when e.g. the hbase tests don't. Just switch to a non-concurrent int (to keep the cleanup in @After) so it's obviously not threadsafe when future maintainers look at it.
(sorry for the thrashing)
- Sean Busbey
On Nov. 4, 2013, 11 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2013, 11 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28173
-----------------------------------------------------------
Ship it!
Thanks again for all the hard work Philip!
Just a couple of minor things. Sorry I missed some of them in a previous review. I don't anticipate anything after these.
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment54808>
Recommend using Configuration.getLong instead of parsing yourself.
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment54809>
Same as above, re Configuration.getLong
src/java/org/apache/sqoop/accumulo/AccumuloUtil.java
<https://reviews.apache.org/r/8559/#comment54811>
Just a note to myself to file a follow on issue for this testing flag in Accumulo and HBase.
They should probably be removed. If not, they should be package protected, since they're only needed in testing. also they should have a thread safety warning.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54813>
Missed an updated file that references AccumuloUtil for constants?
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54817>
Need to also pass MAX_LATENCY and BATCH_SIZE through to Configuration.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54815>
same as above, should be AccumuloUtil
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54814>
Same as above, should be AccumuloUtil
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54816>
same as above, should be AccumuloUtil
src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java
<https://reviews.apache.org/r/8559/#comment54812>
This will make sure we only attempt setupCluster once concurrently, but it won't make parallel tests wait for the cluster to finish.
Why don't we mimic the HBase tests, and just spin up and take down the mini cluster for each test. Does that take too long?
If a minicluster per test does take too long, I'd say it's not worth building the complexity to handle parallel tests correctly when e.g. the hbase tests don't. Just switch to a non-concurrent int (to keep the cleanup in @After) so it's obviously not threadsafe when future maintainers look at it.
(sorry for the thrashing)
- Sean Busbey
On Nov. 4, 2013, 11 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2013, 11 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 5, 2013, 8:13 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Newest review changes.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 13637b5
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 1ffa40f
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 9230f82
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 4, 2013, 11 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Updated with latest fixes.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 13637b5
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 1ffa40f
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 9230f82
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, lines 131-141
> > <https://reviews.apache.org/r/8559/diff/6/?file=373821#file373821line131>
> >
> > Just curious, is there a reason you went for throwing an error rather than using the equivalent of having EMPTY_BYTES for these, the way the convenience methods in Mutation (http://bit.ly/16VR93I) handle a missing visibility?
>
> Philip Grim wrote:
> Do we really want to allow an import to a blank column family? Does anyone do that? Does Accumulo even allow it? Personally, I don't see a use case for it even if Accumulo did allow it.
>
> Similarly, how can we import data without setting which column in the source data is the unique key? What would we do as the behavior? I suppose a one-up or a hash of the fields or something, but again, I'm not sure of the usefulness of that.
>
> Finally, I intended the behavior of this import to be consistent with the behavior of the HBase import, which also requires these to be set (or at least, they did when I first wrote this code a year ago).
consistent with existing HBase is good, and the current trunk still requires both.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28072
-----------------------------------------------------------
On Nov. 4, 2013, 11 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2013, 11 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java, line 53
> > <https://reviews.apache.org/r/8559/diff/6/?file=373830#file373830line53>
> >
> > Could you add a test class that ensures that when we use ACCUMULO_TEST_MODE, nothing is written to the accumulo instance specified in the connection arguments?
I'm getting rid of Accumulo test mode as a command line parameter. This appears to cause confusion and was only added in the first place because when I first wrote this code, the MAC wasn't available, and the methodology for accessing an instance of MockAccumulo vs. accessing a real cluster was different. Since the access methodology for accessing a MAC is the same as accessing a real cluster (since it IS a real cluster for the purposes of Instance creation) being able to specify this isn't necessary any more.
The test mode itself was only ever intended to be used by the JUnit tests, not by an actual user. So, off it goes. The JUnit test can create the MAC and pass the connection info as if it was a real cluster using the correct command line arguments for that purpose.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java, lines 145-155
> > <https://reviews.apache.org/r/8559/diff/6/?file=373830#file373830line145>
> >
> > It looks like Sqoop uses JUnit 4.11 to run tests, which means they could be run in parallel (though I don't know if Sqoop is usually built this way).
> >
> > At a minimum, this needs a strong warning about not being thread safe.
> >
> > it should probably be rewritten with a lock. and maybe an AtomicInteger, which you could use to do reference counting so that stopping can happen in the tearDown method.
> >
> > (
> > http://bit.ly/182jsOe
> > and
> > http://bit.ly/1iCYeeE
> > )
While the JUnit 4 library is being used, it still looks like JUnit 3 semantics are actually used to run the tests, because all of the tests inherit from TestCase. All of the reading I've done indicate that doing that throws JUnit into doing things the old way. Still, you're correct that it's not thread safe.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, lines 131-141
> > <https://reviews.apache.org/r/8559/diff/6/?file=373821#file373821line131>
> >
> > Just curious, is there a reason you went for throwing an error rather than using the equivalent of having EMPTY_BYTES for these, the way the convenience methods in Mutation (http://bit.ly/16VR93I) handle a missing visibility?
Do we really want to allow an import to a blank column family? Does anyone do that? Does Accumulo even allow it? Personally, I don't see a use case for it even if Accumulo did allow it.
Similarly, how can we import data without setting which column in the source data is the unique key? What would we do as the behavior? I suppose a one-up or a hash of the fields or something, but again, I'm not sure of the usefulness of that.
Finally, I intended the behavior of this import to be consistent with the behavior of the HBase import, which also requires these to be set (or at least, they did when I first wrote this code a year ago).
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, lines 53-96
> > <https://reviews.apache.org/r/8559/diff/6/?file=373821#file373821line53>
> >
> > Could these go in a central place? Either in a accumulo.Constants calss or AccumuloUtil?
Moved to AccumuloUtil
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, lines 147-178
> > <https://reviews.apache.org/r/8559/diff/6/?file=373821#file373821line147>
> >
> > When ACCUMULO_TEST_MODE is set, won't all of this error out?
> >
> > In the event that the job set up uses the test mode to create a MAC (and sets the ZK and instance names correctly), then the line getting the test mode option isn't needed.
Removed ACCUMULO_TEST_MODE.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java, lines 139-154
> > <https://reviews.apache.org/r/8559/diff/6/?file=373826#file373826line139>
> >
> > Doesn't this fail in ACCUMULO_TEST_MODE?
> >
> > This looks like the place where we should set up the MAC, presuming we can overwrite the Accumulo connection information.
Removed ACCUMULO_TEST_MODE.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/tool/BaseSqoopTool.java, lines 1524-1539
> > <https://reviews.apache.org/r/8559/diff/6/?file=373828#file373828line1524>
> >
> > If I specified ACCUMULO_TEST_MODE, I don't need to provide these options, yes?
Removed ACCUMULO_TEST_MODE.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28072
-----------------------------------------------------------
On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:22 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, lines 131-141
> > <https://reviews.apache.org/r/8559/diff/6/?file=373821#file373821line131>
> >
> > Just curious, is there a reason you went for throwing an error rather than using the equivalent of having EMPTY_BYTES for these, the way the convenience methods in Mutation (http://bit.ly/16VR93I) handle a missing visibility?
>
> Philip Grim wrote:
> Do we really want to allow an import to a blank column family? Does anyone do that? Does Accumulo even allow it? Personally, I don't see a use case for it even if Accumulo did allow it.
>
> Similarly, how can we import data without setting which column in the source data is the unique key? What would we do as the behavior? I suppose a one-up or a hash of the fields or something, but again, I'm not sure of the usefulness of that.
>
> Finally, I intended the behavior of this import to be consistent with the behavior of the HBase import, which also requires these to be set (or at least, they did when I first wrote this code a year ago).
consistent with existing HBase is good, and the current trunk still requires both.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28072
-----------------------------------------------------------
On Nov. 4, 2013, 11 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2013, 11 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java, line 53
> > <https://reviews.apache.org/r/8559/diff/6/?file=373830#file373830line53>
> >
> > Could you add a test class that ensures that when we use ACCUMULO_TEST_MODE, nothing is written to the accumulo instance specified in the connection arguments?
I'm getting rid of Accumulo test mode as a command line parameter. This appears to cause confusion and was only added in the first place because when I first wrote this code, the MAC wasn't available, and the methodology for accessing an instance of MockAccumulo vs. accessing a real cluster was different. Since the access methodology for accessing a MAC is the same as accessing a real cluster (since it IS a real cluster for the purposes of Instance creation) being able to specify this isn't necessary any more.
The test mode itself was only ever intended to be used by the JUnit tests, not by an actual user. So, off it goes. The JUnit test can create the MAC and pass the connection info as if it was a real cluster using the correct command line arguments for that purpose.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java, lines 145-155
> > <https://reviews.apache.org/r/8559/diff/6/?file=373830#file373830line145>
> >
> > It looks like Sqoop uses JUnit 4.11 to run tests, which means they could be run in parallel (though I don't know if Sqoop is usually built this way).
> >
> > At a minimum, this needs a strong warning about not being thread safe.
> >
> > it should probably be rewritten with a lock. and maybe an AtomicInteger, which you could use to do reference counting so that stopping can happen in the tearDown method.
> >
> > (
> > http://bit.ly/182jsOe
> > and
> > http://bit.ly/1iCYeeE
> > )
While the JUnit 4 library is being used, it still looks like JUnit 3 semantics are actually used to run the tests, because all of the tests inherit from TestCase. All of the reading I've done indicate that doing that throws JUnit into doing things the old way. Still, you're correct that it's not thread safe.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, lines 131-141
> > <https://reviews.apache.org/r/8559/diff/6/?file=373821#file373821line131>
> >
> > Just curious, is there a reason you went for throwing an error rather than using the equivalent of having EMPTY_BYTES for these, the way the convenience methods in Mutation (http://bit.ly/16VR93I) handle a missing visibility?
Do we really want to allow an import to a blank column family? Does anyone do that? Does Accumulo even allow it? Personally, I don't see a use case for it even if Accumulo did allow it.
Similarly, how can we import data without setting which column in the source data is the unique key? What would we do as the behavior? I suppose a one-up or a hash of the fields or something, but again, I'm not sure of the usefulness of that.
Finally, I intended the behavior of this import to be consistent with the behavior of the HBase import, which also requires these to be set (or at least, they did when I first wrote this code a year ago).
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, lines 53-96
> > <https://reviews.apache.org/r/8559/diff/6/?file=373821#file373821line53>
> >
> > Could these go in a central place? Either in a accumulo.Constants calss or AccumuloUtil?
Moved to AccumuloUtil
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, lines 147-178
> > <https://reviews.apache.org/r/8559/diff/6/?file=373821#file373821line147>
> >
> > When ACCUMULO_TEST_MODE is set, won't all of this error out?
> >
> > In the event that the job set up uses the test mode to create a MAC (and sets the ZK and instance names correctly), then the line getting the test mode option isn't needed.
Removed ACCUMULO_TEST_MODE.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java, lines 139-154
> > <https://reviews.apache.org/r/8559/diff/6/?file=373826#file373826line139>
> >
> > Doesn't this fail in ACCUMULO_TEST_MODE?
> >
> > This looks like the place where we should set up the MAC, presuming we can overwrite the Accumulo connection information.
Removed ACCUMULO_TEST_MODE.
> On Nov. 2, 2013, 5:04 p.m., Sean Busbey wrote:
> > src/java/org/apache/sqoop/tool/BaseSqoopTool.java, lines 1524-1539
> > <https://reviews.apache.org/r/8559/diff/6/?file=373828#file373828line1524>
> >
> > If I specified ACCUMULO_TEST_MODE, I don't need to provide these options, yes?
Removed ACCUMULO_TEST_MODE.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28072
-----------------------------------------------------------
On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:22 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28072
-----------------------------------------------------------
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment54619>
Could these go in a central place? Either in a accumulo.Constants calss or AccumuloUtil?
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment54617>
Just curious, is there a reason you went for throwing an error rather than using the equivalent of having EMPTY_BYTES for these, the way the convenience methods in Mutation (http://bit.ly/16VR93I) handle a missing visibility?
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment54618>
When ACCUMULO_TEST_MODE is set, won't all of this error out?
In the event that the job set up uses the test mode to create a MAC (and sets the ZK and instance names correctly), then the line getting the test mode option isn't needed.
src/java/org/apache/sqoop/accumulo/MutationTransformer.java
<https://reviews.apache.org/r/8559/#comment54620>
Recommend Iterable<Mutation> instead of List, so that other implementations have an easier time not materializing the whole list in memory.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54621>
Doesn't this fail in ACCUMULO_TEST_MODE?
This looks like the place where we should set up the MAC, presuming we can overwrite the Accumulo connection information.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment54622>
If I specified ACCUMULO_TEST_MODE, I don't need to provide these options, yes?
src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java
<https://reviews.apache.org/r/8559/#comment54624>
Could you add a test class that ensures that when we use ACCUMULO_TEST_MODE, nothing is written to the accumulo instance specified in the connection arguments?
src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java
<https://reviews.apache.org/r/8559/#comment54623>
It looks like Sqoop uses JUnit 4.11 to run tests, which means they could be run in parallel (though I don't know if Sqoop is usually built this way).
At a minimum, this needs a strong warning about not being thread safe.
it should probably be rewritten with a lock. and maybe an AtomicInteger, which you could use to do reference counting so that stopping can happen in the tearDown method.
(
http://bit.ly/182jsOe
and
http://bit.ly/1iCYeeE
)
- Sean Busbey
On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:22 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review28072
-----------------------------------------------------------
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment54619>
Could these go in a central place? Either in a accumulo.Constants calss or AccumuloUtil?
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment54617>
Just curious, is there a reason you went for throwing an error rather than using the equivalent of having EMPTY_BYTES for these, the way the convenience methods in Mutation (http://bit.ly/16VR93I) handle a missing visibility?
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment54618>
When ACCUMULO_TEST_MODE is set, won't all of this error out?
In the event that the job set up uses the test mode to create a MAC (and sets the ZK and instance names correctly), then the line getting the test mode option isn't needed.
src/java/org/apache/sqoop/accumulo/MutationTransformer.java
<https://reviews.apache.org/r/8559/#comment54620>
Recommend Iterable<Mutation> instead of List, so that other implementations have an easier time not materializing the whole list in memory.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment54621>
Doesn't this fail in ACCUMULO_TEST_MODE?
This looks like the place where we should set up the MAC, presuming we can overwrite the Accumulo connection information.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/8559/#comment54622>
If I specified ACCUMULO_TEST_MODE, I don't need to provide these options, yes?
src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java
<https://reviews.apache.org/r/8559/#comment54624>
Could you add a test class that ensures that when we use ACCUMULO_TEST_MODE, nothing is written to the accumulo instance specified in the connection arguments?
src/test/org/apache/sqoop/accumulo/AccumuloTestCase.java
<https://reviews.apache.org/r/8559/#comment54623>
It looks like Sqoop uses JUnit 4.11 to run tests, which means they could be run in parallel (though I don't know if Sqoop is usually built this way).
At a minimum, this needs a strong warning about not being thread safe.
it should probably be rewritten with a lock. and maybe an AtomicInteger, which you could use to do reference counting so that stopping can happen in the tearDown method.
(
http://bit.ly/182jsOe
and
http://bit.ly/1iCYeeE
)
- Sean Busbey
On Oct. 29, 2013, 9:22 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:22 p.m.)
>
>
> Review request for accumulo, Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> 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 c5130ae
> src/docs/user/accumulo-args.txt PRE-CREATION
> src/docs/user/accumulo.txt PRE-CREATION
> src/java/org/apache/sqoop/SqoopOptions.java 13637b5
> 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 1ffa40f
> 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 9230f82
> src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
> 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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Nov. 4, 2013, 11 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Updated with latest fixes.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 13637b5
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 1ffa40f
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 9230f82
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Oct. 29, 2013, 9:22 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Updated with fixes.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 13637b5
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 1ffa40f
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 9230f82
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Oct. 29, 2013, 9:22 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
Updated with fixes.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 13637b5
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 1ffa40f
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 9230f82
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Oct. 23, 2013, 9:24 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
New patch uploaded. Updated to latest trunk.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 13637b5
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 1ffa40f
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 9230f82
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On July 19, 2013, 12:33 a.m., Jarek Cecho wrote:
> > Hi Philip,
> > please accept my deepest apologies for not following up here for so long. I think that we should finally get this in and I was wondering if you are still interested in working on this JIRA?
> >
> > Jarcec
Jarek,
I'll take a look at what needs to be done to move it forward to 1.4.5 as soon as I get a chance.
Philip
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review23466
-----------------------------------------------------------
On Jan. 17, 2013, 3:30 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> Repository: sqoop-trunk
>
>
> 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
>
>
Re: Review Request 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On July 19, 2013, 12:33 a.m., Jarek Cecho wrote:
> > Hi Philip,
> > please accept my deepest apologies for not following up here for so long. I think that we should finally get this in and I was wondering if you are still interested in working on this JIRA?
> >
> > Jarcec
Jarek,
I'll take a look at what needs to be done to move it forward to 1.4.5 as soon as I get a chance.
Philip
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review23466
-----------------------------------------------------------
On Jan. 17, 2013, 3:30 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> Repository: sqoop-trunk
>
>
> 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
>
>
Re: Review Request 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review23466
-----------------------------------------------------------
Hi Philip,
please accept my deepest apologies for not following up here for so long. I think that we should finally get this in and I was wondering if you are still interested in working on this JIRA?
Jarcec
- Jarek Cecho
On Jan. 17, 2013, 3:30 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> Repository: sqoop-trunk
>
>
> 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
>
>
Re: Review Request 8559: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/
-----------------------------------------------------------
(Updated Oct. 23, 2013, 9:24 p.m.)
Review request for accumulo, Sqoop and Jarek Cecho.
Changes
-------
New patch uploaded. Updated to latest trunk.
Repository: sqoop-trunk
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 c5130ae
src/docs/user/accumulo-args.txt PRE-CREATION
src/docs/user/accumulo.txt PRE-CREATION
src/java/org/apache/sqoop/SqoopOptions.java 13637b5
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 1ffa40f
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 9230f82
src/java/org/apache/sqoop/tool/ImportTool.java fbbde1d
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 8559: Add Accumulo support to Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review23466
-----------------------------------------------------------
Hi Philip,
please accept my deepest apologies for not following up here for so long. I think that we should finally get this in and I was wondering if you are still interested in working on this JIRA?
Jarcec
- Jarek Cecho
On Jan. 17, 2013, 3:30 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> Repository: sqoop-trunk
>
>
> 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
>
>
Re: Review Request: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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
Re: Review Request: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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
Re: Review Request: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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 Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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 Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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 Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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 Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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 Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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 Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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 Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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 Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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 Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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 Sqoop
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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.
Changes
-------
Updated to address redundant table creates, fix race condition, and clean up comment errors.
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)
-----
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
Re: Review Request: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Dec. 12, 2012, 11:37 p.m., kturner wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, line 134
> > <https://reviews.apache.org/r/8559/diff/1/?file=237547#file237547line134>
> >
> > The buffer seems a bit large to me and the time seems small.
What values would you recommend? Should they be configurable with command-line options, perhaps?
> On Dec. 12, 2012, 11:37 p.m., kturner wrote:
> > src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java, line 132
> > <https://reviews.apache.org/r/8559/diff/1/?file=237552#file237552line132>
> >
> > The comment seems a bit off :)
Cut and paste will be the death of me.
> On Dec. 12, 2012, 11:37 p.m., kturner wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, line 132
> > <https://reviews.apache.org/r/8559/diff/1/?file=237547#file237547line132>
> >
> > Recommend doing the following to avoid race condition where another thread creates table between exists check and call to create
> >
> > if (!conn.tableOperations().exists(tableName))
> > try{conn.tableOperations().create(tableName);} catch (TableExistsException tee) {}
> >
> > Do not need exists check, but it will run faster with it in the case where the table usually exists.
> >
Looking back, this create is actually redundant, as the other one should already have taken care of it in the job setup.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review14403
-----------------------------------------------------------
On Dec. 12, 2012, 9:50 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 9:50 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
>
>
Re: Review Request: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Dec. 12, 2012, 11:37 p.m., kturner wrote:
> > src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java, line 158
> > <https://reviews.apache.org/r/8559/diff/1/?file=237552#file237552line158>
> >
> > another race condition... table could have been created since exists was called
I'll fix it here, and delete the other one.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review14403
-----------------------------------------------------------
On Dec. 12, 2012, 9:50 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 9:50 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
>
>
Re: Review Request: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Dec. 12, 2012, 11:37 p.m., kturner wrote:
> > src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java, line 158
> > <https://reviews.apache.org/r/8559/diff/1/?file=237552#file237552line158>
> >
> > another race condition... table could have been created since exists was called
I'll fix it here, and delete the other one.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review14403
-----------------------------------------------------------
On Dec. 12, 2012, 9:50 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 9:50 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
>
>
Re: Review Request: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
> On Dec. 12, 2012, 11:37 p.m., kturner wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, line 134
> > <https://reviews.apache.org/r/8559/diff/1/?file=237547#file237547line134>
> >
> > The buffer seems a bit large to me and the time seems small.
What values would you recommend? Should they be configurable with command-line options, perhaps?
> On Dec. 12, 2012, 11:37 p.m., kturner wrote:
> > src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java, line 132
> > <https://reviews.apache.org/r/8559/diff/1/?file=237552#file237552line132>
> >
> > The comment seems a bit off :)
Cut and paste will be the death of me.
> On Dec. 12, 2012, 11:37 p.m., kturner wrote:
> > src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java, line 132
> > <https://reviews.apache.org/r/8559/diff/1/?file=237547#file237547line132>
> >
> > Recommend doing the following to avoid race condition where another thread creates table between exists check and call to create
> >
> > if (!conn.tableOperations().exists(tableName))
> > try{conn.tableOperations().create(tableName);} catch (TableExistsException tee) {}
> >
> > Do not need exists check, but it will run faster with it in the case where the table usually exists.
> >
Looking back, this create is actually redundant, as the other one should already have taken care of it in the job setup.
- Philip
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review14403
-----------------------------------------------------------
On Dec. 12, 2012, 9:50 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 9:50 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
>
>
Re: Review Request: Add Accumulo support to Sqoop
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review14403
-----------------------------------------------------------
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment30637>
Recommend doing the following to avoid race condition where another thread creates table between exists check and call to create
if (!conn.tableOperations().exists(tableName))
try{conn.tableOperations().create(tableName);} catch (TableExistsException tee) {}
Do not need exists check, but it will run faster with it in the case where the table usually exists.
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment30643>
The buffer seems a bit large to me and the time seems small.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment30639>
The comment seems a bit off :)
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment30644>
another race condition... table could have been created since exists was called
- kturner
On Dec. 12, 2012, 9:50 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 9:50 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
>
>
Re: Review Request: Add Accumulo support to Sqoop
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8559/#review14403
-----------------------------------------------------------
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment30637>
Recommend doing the following to avoid race condition where another thread creates table between exists check and call to create
if (!conn.tableOperations().exists(tableName))
try{conn.tableOperations().create(tableName);} catch (TableExistsException tee) {}
Do not need exists check, but it will run faster with it in the case where the table usually exists.
src/java/org/apache/sqoop/accumulo/AccumuloMutationProcessor.java
<https://reviews.apache.org/r/8559/#comment30643>
The buffer seems a bit large to me and the time seems small.
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment30639>
The comment seems a bit off :)
src/java/org/apache/sqoop/mapreduce/AccumuloImportJob.java
<https://reviews.apache.org/r/8559/#comment30644>
another race condition... table could have been created since exists was called
- kturner
On Dec. 12, 2012, 9:50 p.m., Philip Grim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8559/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 9:50 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
>
>
Re: Review Request: Add Accumulo support to Sqoop
Posted by Philip Grim <ph...@insufficient-light.com>.
-----------------------------------------------------------
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.
Changes
-------
Updated to address redundant table creates, fix race condition, and clean up comment errors.
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)
-----
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