accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Grim" <p...@insufficient-light.com>
Subject Re: Review Request 8559: Add Accumulo support to Sqoop
Date Mon, 04 Nov 2013 22:59:11 GMT


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


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