sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sean Busbey" <s...@manvsbeard.com>
Subject Re: Review Request 8559: Add Accumulo support to Sqoop
Date Sat, 02 Nov 2013 17:04:34 GMT

This is an automatically generated e-mail. To reply, visit:


    Could these go in a central place? Either in a accumulo.Constants calss or AccumuloUtil?


    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?


    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.


    Recommend Iterable<Mutation> instead of List, so that other implementations have
an easier time not materializing the whole list in memory.


    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.


    If I specified ACCUMULO_TEST_MODE, I don't need to provide these options, yes?


    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?


    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.

- 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

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