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

Switch to Threaded View
Accumulo >> mail # dev >> Re: Review Request 8559: Add Accumulo support to Sqoop


Copy link to this message
-
Re: Review Request 8559: Add Accumulo support to Sqoop

-----------------------------------------------------------
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